| Message ID | 1490856383-31560-15-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
On Thu, 30 Mar 2017 15:46:00 +0900 Masahiro Yamada <yamada.masahiro@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, 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). For PCI users, you explicitly set the NAND_ECC_MAXIMIZE flag, so it shouldn't be a problem (you're still backward compatible). > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Acked-by: Rob Herring <robh@kernel.org> > --- > > Changes in v3: None > Changes in v2: > - Add available values in the binding document > > Documentation/devicetree/bindings/mtd/denali-nand.txt | 6 ++++++ > drivers/mtd/nand/denali.c | 18 ++++++++++++++++-- > drivers/mtd/nand/denali_pci.c | 1 + > 3 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt > index 25313c7..647618e 100644 > --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt > @@ -11,6 +11,12 @@ Optional properties: > - nand-ecc-step-size: must be 512 or 1024. If not specified, default to: > 512 for "altr,socfpga-denali-nand" > see nand.txt for details. > + - nand-ecc-strength: see nand.txt for details. Available values are: > + 8, 15 for "altr,socfpga-denali-nand" > + - nand-ecc-maximize: see nand.txt for details > + > +Note: > +Either nand-ecc-strength or nand-ecc-maximize should be specified. > > The device tree may optionally contain sub-nodes describing partitions of the > address space. See partition.txt for more detail. > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > index ce87b95..2f796e3 100644 > --- a/drivers/mtd/nand/denali.c > +++ b/drivers/mtd/nand/denali.c > @@ -1641,9 +1641,23 @@ int denali_init(struct denali_nand_info *denali) > goto failed_req_irq; > } > > - ret = denali_set_max_ecc_strength(denali); > - if (ret) > + if (!chip->ecc.strength && !(chip->ecc.options & NAND_ECC_MAXIMIZE)) { > + dev_info(denali->dev, > + "No ECC strength strategy is specified. Maximizing ECC strength\n"); > + chip->ecc.options |= NAND_ECC_MAXIMIZE; > + } > + > + if (chip->ecc.options & NAND_ECC_MAXIMIZE) { > + ret = denali_set_max_ecc_strength(denali); > + if (ret) > + goto failed_req_irq; > + } else if (!(denali->ecc_strength_avail & BIT(chip->ecc.strength))) { > + dev_err(denali->dev, > + "ECC strength %d is not supported on this controller.\n", > + chip->ecc.strength); > + ret = -EINVAL; > goto failed_req_irq; > + } > > chip->ecc.bytes = denali_calc_ecc_bytes(chip->ecc.size, > chip->ecc.strength); > diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c > index a1ee9f8..a39682a5 100644 > --- a/drivers/mtd/nand/denali_pci.c > +++ b/drivers/mtd/nand/denali_pci.c > @@ -87,6 +87,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) > > denali->ecc_strength_avail = BIT(15) | BIT(8); > denali->caps |= DENALI_CAP_ECC_SIZE_512; > + denali->nand.ecc.options |= NAND_ECC_MAXIMIZE; > > ret = denali_init(denali); > if (ret)
Hi Boris, 2017-03-30 23:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Thu, 30 Mar 2017 15:46:00 +0900 > Masahiro Yamada <yamada.masahiro@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. 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.
On Fri, 31 Mar 2017 14:06:32 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > 2017-03-30 23:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > On Thu, 30 Mar 2017 15:46:00 +0900 > > Masahiro Yamada <yamada.masahiro@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.
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? > 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.
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.
Hi Boris, 2017-04-10 1:33 GMT+09:00 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. After I fiddle with this generic approach for a while, I started to feel like giving up. I wonder if we really want over-implementation for covering _theoretically_ possible cases. In practice, there are not so many ECC settings possible on a single controller. As for Denali IP, it would be theoretically possible to instantiate multiple ECC engines. However, in practice, there is no sensible reason to do so. At least, I do not know any real chip to support that. So, I'd like to simplify the logic for Denali. - Support either 512 or 1024 ECC size. If there is (ever) a controller that supports both, 1024 should be chosen. - ECC strength is not specified via DT, it is simply maximized. This simplifies the logic much and I believe this is enough. One more reason is, as we talked before, we need to match ECC setting between Linux and firmware (boot-loader), so anyway we end up with using a fixed setting specified by DT.
Hi Masahiro, On Tue, 11 Apr 2017 15:19:21 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > > 2017-04-10 1:33 GMT+09:00 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. > > > After I fiddle with this generic approach for a while, > I started to feel like giving up. I don't get it. What was the problem with my initial suggestion (the denali specific one, not the generic approach)? You proposed to make it generic, which, I agree, is a bit more complicated. > > I wonder if we really want over-implementation > for covering _theoretically_ possible cases. Okay, one more theoretical case I'd like to expose: you have board design with different NAND parts which have different ECC requirements. If you were about to describe the exact ECC strength you want for each board you'll have to have different DTs. Maximizing the ECC strength would still work, but what if the MTD user needs some OOB bytes (like is the case with JFFS2) and ECC maximization reserved all of the available bytes? The other reason I prefer to have the drivers automatically guessing what's appropriate is because then you don't have to care when writing your DT. > > In practice, there are not so many ECC settings possible > on a single controller. > > As for Denali IP, it would be theoretically possible to instantiate > multiple ECC engines. However, in practice, there is no sensible > reason to do so. At least, I do not know any real chip to support that. > > So, I'd like to simplify the logic for Denali. > > - Support either 512 or 1024 ECC size. > If there is (ever) a controller that supports both, > 1024 should be chosen. > > - ECC strength is not specified via DT, it is simply maximized. > > This simplifies the logic much and I believe this is enough. > > One more reason is, as we talked before, > we need to match ECC setting between Linux and firmware (boot-loader), If the bootloader implements the same logic it should match. > so anyway we end up with using a fixed setting specified by DT. > Really, I don't see what's the problem with the function I proposed, but I'm willing to make a concession. Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize mandatory so that if someone ever needs to support the 'match NAND requirements' feature we won't have to add a vendor specific property like this one [1]. Are you fine with that? [1]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/gpmi-nand.txt#L20
Hi Boris, 2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > Hi Masahiro, > > On Tue, 11 Apr 2017 15:19:21 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> Hi Boris, >> >> >> >> 2017-04-10 1:33 GMT+09:00 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. >> >> >> After I fiddle with this generic approach for a while, >> I started to feel like giving up. > > I don't get it. What was the problem with my initial suggestion (the > denali specific one, not the generic approach)? You proposed to make it > generic, which, I agree, is a bit more complicated. > >> >> I wonder if we really want over-implementation >> for covering _theoretically_ possible cases. > > Okay, one more theoretical case I'd like to expose: you have board > design with different NAND parts which have different ECC requirements. > If you were about to describe the exact ECC strength you want for each > board you'll have to have different DTs. In this case, fixed ecc-strength in DT is not feasible. > Maximizing the ECC strength > would still work, but what if the MTD user needs some OOB bytes (like > is the case with JFFS2) and ECC maximization reserved all of the > available bytes? JFFS2 needs some bytes in oob-free area for the clean marker. You are right. This implies NAND_ECC_MAXIMIZE is not very useful. We do not know whether we have enough space left in oob, or not. > The other reason I prefer to have the drivers automatically guessing > what's appropriate is because then you don't have to care when writing > your DT. > >> >> In practice, there are not so many ECC settings possible >> on a single controller. >> >> As for Denali IP, it would be theoretically possible to instantiate >> multiple ECC engines. However, in practice, there is no sensible >> reason to do so. At least, I do not know any real chip to support that. >> >> So, I'd like to simplify the logic for Denali. >> >> - Support either 512 or 1024 ECC size. >> If there is (ever) a controller that supports both, >> 1024 should be chosen. >> >> - ECC strength is not specified via DT, it is simply maximized. >> >> This simplifies the logic much and I believe this is enough. >> >> One more reason is, as we talked before, >> we need to match ECC setting between Linux and firmware (boot-loader), > > If the bootloader implements the same logic it should match. > >> so anyway we end up with using a fixed setting specified by DT. >> > > Really, I don't see what's the problem with the function I proposed, > but I'm willing to make a concession. > Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize > mandatory so that if someone ever needs to support the 'match NAND > requirements' feature we won't have to add a vendor specific property > like this one [1]. > > Are you fine with that? No. This requirement seems too strong. At least, it is a problem for non-DT platforms. If a driver provides ECC engine caps info, perhaps ECC maximizing could be a generalized helper function as well. I am trying this still.
On Fri, 14 Apr 2017 16:57:23 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > 2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > Hi Masahiro, > > > > On Tue, 11 Apr 2017 15:19:21 +0900 > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > >> Hi Boris, > >> > >> > >> > >> 2017-04-10 1:33 GMT+09:00 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. > >> > >> > >> After I fiddle with this generic approach for a while, > >> I started to feel like giving up. > > > > I don't get it. What was the problem with my initial suggestion (the > > denali specific one, not the generic approach)? You proposed to make it > > generic, which, I agree, is a bit more complicated. > > > >> > >> I wonder if we really want over-implementation > >> for covering _theoretically_ possible cases. > > > > Okay, one more theoretical case I'd like to expose: you have board > > design with different NAND parts which have different ECC requirements. > > If you were about to describe the exact ECC strength you want for each > > board you'll have to have different DTs. > > In this case, fixed ecc-strength in DT is not feasible. > > > Maximizing the ECC strength > > would still work, but what if the MTD user needs some OOB bytes (like > > is the case with JFFS2) and ECC maximization reserved all of the > > available bytes? > > JFFS2 needs some bytes in oob-free area for the clean marker. > You are right. > This implies NAND_ECC_MAXIMIZE is not very useful. > We do not know whether we have enough space left in oob, or not. > > > > > The other reason I prefer to have the drivers automatically guessing > > what's appropriate is because then you don't have to care when writing > > your DT. > > > >> > >> In practice, there are not so many ECC settings possible > >> on a single controller. > >> > >> As for Denali IP, it would be theoretically possible to instantiate > >> multiple ECC engines. However, in practice, there is no sensible > >> reason to do so. At least, I do not know any real chip to support that. > >> > >> So, I'd like to simplify the logic for Denali. > >> > >> - Support either 512 or 1024 ECC size. > >> If there is (ever) a controller that supports both, > >> 1024 should be chosen. > >> > >> - ECC strength is not specified via DT, it is simply maximized. > >> > >> This simplifies the logic much and I believe this is enough. > >> > >> One more reason is, as we talked before, > >> we need to match ECC setting between Linux and firmware (boot-loader), > > > > If the bootloader implements the same logic it should match. > > > >> so anyway we end up with using a fixed setting specified by DT. > >> > > > > Really, I don't see what's the problem with the function I proposed, > > but I'm willing to make a concession. > > Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize > > mandatory so that if someone ever needs to support the 'match NAND > > requirements' feature we won't have to add a vendor specific property > > like this one [1]. > > > > Are you fine with that? > > No. This requirement seems too strong. Hm, can you give more details? All I want is a solution where we can later support the feature I'm asking without adding a extra DT property, and, in order to do that we must make sure the case you want to support as a first step are explicitly requested in the DT. It's as simple as: if ((!ecc->strength || !ecc->size) && !(ecc->options & NAND_ECC_MAXIMIZE)) return -ENOTSUPP; > At least, it is a problem for non-DT platforms. Well, for non-DT platforms you have to keep ECC maximization anyway, otherwise you're not backward compatible. > > > If a driver provides ECC engine caps info, > perhaps ECC maximizing could be a generalized helper function as well. I don't get it. I thought the generic helper was too hard to implement. Now you want to add a new functionality. I'm not against this idea, but maybe it's easier to provide a denali specific implementation before tackling the generic one.
Hi Boris, 2017-04-14 17:19 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Fri, 14 Apr 2017 16:57:23 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> Hi Boris, >> >> >> 2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> > Hi Masahiro, >> > >> > On Tue, 11 Apr 2017 15:19:21 +0900 >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> > >> >> Hi Boris, >> >> >> >> >> >> >> >> 2017-04-10 1:33 GMT+09:00 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. >> >> >> >> >> >> After I fiddle with this generic approach for a while, >> >> I started to feel like giving up. >> > >> > I don't get it. What was the problem with my initial suggestion (the >> > denali specific one, not the generic approach)? You proposed to make it >> > generic, which, I agree, is a bit more complicated. >> > >> >> >> >> I wonder if we really want over-implementation >> >> for covering _theoretically_ possible cases. >> > >> > Okay, one more theoretical case I'd like to expose: you have board >> > design with different NAND parts which have different ECC requirements. >> > If you were about to describe the exact ECC strength you want for each >> > board you'll have to have different DTs. >> >> In this case, fixed ecc-strength in DT is not feasible. >> >> > Maximizing the ECC strength >> > would still work, but what if the MTD user needs some OOB bytes (like >> > is the case with JFFS2) and ECC maximization reserved all of the >> > available bytes? >> >> JFFS2 needs some bytes in oob-free area for the clean marker. >> You are right. >> This implies NAND_ECC_MAXIMIZE is not very useful. >> We do not know whether we have enough space left in oob, or not. >> >> >> >> > The other reason I prefer to have the drivers automatically guessing >> > what's appropriate is because then you don't have to care when writing >> > your DT. >> > >> >> >> >> In practice, there are not so many ECC settings possible >> >> on a single controller. >> >> >> >> As for Denali IP, it would be theoretically possible to instantiate >> >> multiple ECC engines. However, in practice, there is no sensible >> >> reason to do so. At least, I do not know any real chip to support that. >> >> >> >> So, I'd like to simplify the logic for Denali. >> >> >> >> - Support either 512 or 1024 ECC size. >> >> If there is (ever) a controller that supports both, >> >> 1024 should be chosen. >> >> >> >> - ECC strength is not specified via DT, it is simply maximized. >> >> >> >> This simplifies the logic much and I believe this is enough. >> >> >> >> One more reason is, as we talked before, >> >> we need to match ECC setting between Linux and firmware (boot-loader), >> > >> > If the bootloader implements the same logic it should match. >> > >> >> so anyway we end up with using a fixed setting specified by DT. >> >> >> > >> > Really, I don't see what's the problem with the function I proposed, >> > but I'm willing to make a concession. >> > Make the nand-ecc-strength+nand-ecc-step-size or nand-ecc-maximize >> > mandatory so that if someone ever needs to support the 'match NAND >> > requirements' feature we won't have to add a vendor specific property >> > like this one [1]. >> > >> > Are you fine with that? >> >> No. This requirement seems too strong. > > Hm, can you give more details? All I want is a solution where we can > later support the feature I'm asking without adding a extra DT > property, and, in order to do that we must make sure the case you want > to support as a first step are explicitly requested in the DT. > > It's as simple as: > > if ((!ecc->strength || !ecc->size) && > !(ecc->options & NAND_ECC_MAXIMIZE)) > return -ENOTSUPP; If a controller supports only one possible value for nand-ecc-step-size, users have no choice anyway. For UniPhier SoCs, nand-ecc-step-size = <1024>; nand-ecc-strength = <8> or <16> or <24>; But, it is harmless even if we specify nand-ecc-step-size explicitly. So, I do not argue here. >> At least, it is a problem for non-DT platforms. > > Well, for non-DT platforms you have to keep ECC maximization anyway, > otherwise you're not backward compatible. > >> >> >> If a driver provides ECC engine caps info, >> perhaps ECC maximizing could be a generalized helper function as well. > > I don't get it. I thought the generic helper was too hard to implement. > Now you want to add a new functionality. > > I'm not against this idea, but maybe it's easier to provide a denali > specific implementation before tackling the generic one. I think there is a common logic in matching request and maximizing. I could not explain well in my words, so I wrote a patch: http://patchwork.ozlabs.org/patch/752107/ Could you check it?
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt index 25313c7..647618e 100644 --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt @@ -11,6 +11,12 @@ Optional properties: - nand-ecc-step-size: must be 512 or 1024. If not specified, default to: 512 for "altr,socfpga-denali-nand" see nand.txt for details. + - nand-ecc-strength: see nand.txt for details. Available values are: + 8, 15 for "altr,socfpga-denali-nand" + - nand-ecc-maximize: see nand.txt for details + +Note: +Either nand-ecc-strength or nand-ecc-maximize should be specified. The device tree may optionally contain sub-nodes describing partitions of the address space. See partition.txt for more detail. diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index ce87b95..2f796e3 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -1641,9 +1641,23 @@ int denali_init(struct denali_nand_info *denali) goto failed_req_irq; } - ret = denali_set_max_ecc_strength(denali); - if (ret) + if (!chip->ecc.strength && !(chip->ecc.options & NAND_ECC_MAXIMIZE)) { + dev_info(denali->dev, + "No ECC strength strategy is specified. Maximizing ECC strength\n"); + chip->ecc.options |= NAND_ECC_MAXIMIZE; + } + + if (chip->ecc.options & NAND_ECC_MAXIMIZE) { + ret = denali_set_max_ecc_strength(denali); + if (ret) + goto failed_req_irq; + } else if (!(denali->ecc_strength_avail & BIT(chip->ecc.strength))) { + dev_err(denali->dev, + "ECC strength %d is not supported on this controller.\n", + chip->ecc.strength); + ret = -EINVAL; goto failed_req_irq; + } chip->ecc.bytes = denali_calc_ecc_bytes(chip->ecc.size, chip->ecc.strength); diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c index a1ee9f8..a39682a5 100644 --- a/drivers/mtd/nand/denali_pci.c +++ b/drivers/mtd/nand/denali_pci.c @@ -87,6 +87,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) denali->ecc_strength_avail = BIT(15) | BIT(8); denali->caps |= DENALI_CAP_ECC_SIZE_512; + denali->nand.ecc.options |= NAND_ECC_MAXIMIZE; ret = denali_init(denali); if (ret)