Messages in this thread | | | Date | Wed, 30 May 2018 14:23:32 +0530 | From | Abhishek Sahu <> | Subject | Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration |
| |
On 2018-05-30 13:08, Masahiro Yamada wrote: > 2018-05-30 15:21 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>: >> On 2018-05-30 05:58, Masahiro Yamada wrote: >>> >>> Hi. >>> >>> 2018-05-30 4:30 GMT+09:00 Boris Brezillon >>> <boris.brezillon@bootlin.com>: >>>> >>>> On Sat, 26 May 2018 10:42:47 +0200 >>>> Miquel Raynal <miquel.raynal@bootlin.com> wrote: >>>> >>>>> Hi Abhishek, >>>>> >>>>> On Fri, 25 May 2018 17:51:29 +0530, Abhishek Sahu >>>>> <absahu@codeaurora.org> wrote: >>>>> >>>>> > commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check, >>>>> > match, maximize ECC settings") provides generic helpers which >>>>> > drivers can use for setting up ECC parameters. >>>>> > >>>>> > Since same board can have different ECC strength nand chips so >>>>> > following is the logic for setting up ECC strength and ECC step >>>>> > size, which can be used by most of the drivers. >>>>> > >>>>> > 1. If both ECC step size and ECC strength are already set >>>>> > (usually by DT) then just check whether this setting >>>>> > is supported by NAND controller. >>>>> > 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength >>>>> > supported by NAND controller. >>>>> > 3. Otherwise, try to match the ECC step size and ECC strength closest >>>>> > to the chip's requirement. If available OOB size can't fit the chip >>>>> > requirement then select maximum ECC strength which can be fit with >>>>> > available OOB size. >>>>> > >>>>> > This patch introduces nand_ecc_choose_conf function which calls the >>>>> > required helper functions for the above logic. The drivers can use >>>>> > this single function instead of calling the 3 helper functions >>>>> > individually. >>>>> > >>>>> > CC: Masahiro Yamada <yamada.masahiro@socionext.com> >>>>> > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> >>>>> > --- >>>>> > * Changes from v2: >>>>> > >>>>> > 1. Renamed function to nand_ecc_choose_conf. >>>>> > 2. Minor code reorganization to remove warning and 2 function calls >>>>> > for nand_maximize_ecc. >>>>> > >>>>> > * Changes from v1: >>>>> > NEW PATCH >>>>> > >>>>> > drivers/mtd/nand/raw/nand_base.c | 42 >>>>> > ++++++++++++++++++++++++++++++++++++++++ >>>>> > drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++ >>>>> > include/linux/mtd/rawnand.h | 3 +++ >>>>> > 2 files changed, 34 insertions(+) >>>>> > >>>>> > diff --git a/drivers/mtd/nand/raw/nand_base.c >>>>> > b/drivers/mtd/nand/raw/nand_base.c >>>>> > index 72f3a89..e52019d 100644 >>>>> > --- a/drivers/mtd/nand/raw/nand_base.c >>>>> > +++ b/drivers/mtd/nand/raw/nand_base.c >>>>> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip, >>>>> > } >>>>> > EXPORT_SYMBOL_GPL(nand_maximize_ecc); >>>>> > >>>>> > +/** >>>>> > + * nand_ecc_choose_conf - Set the ECC strength and ECC step size >>>>> > + * @chip: nand chip info structure >>>>> > + * @caps: ECC engine caps info structure >>>>> > + * @oobavail: OOB size that the ECC engine can use >>>>> > + * >>>>> > + * Choose the ECC configuration according to following logic >>>>> > + * >>>>> > + * 1. If both ECC step size and ECC strength are already set (usually >>>>> > by DT) >>>>> > + * then check if it is supported by this controller. >>>>> > + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength. >>>>> > + * 3. Otherwise, try to match the ECC step size and ECC strength >>>>> > closest >>>>> > + * to the chip's requirement. If available OOB size can't fit the >>>>> > chip >>>>> > + * requirement then fallback to the maximum ECC step size and ECC >>>>> > strength. >>>>> > + * >>>>> > + * On success, the chosen ECC settings are set. >>>>> > + */ >>>>> > +int nand_ecc_choose_conf(struct nand_chip *chip, >>>>> > + const struct nand_ecc_caps *caps, int oobavail) >>>>> > +{ >>>>> > + if (chip->ecc.size && chip->ecc.strength) >>>>> > + return nand_check_ecc_caps(chip, caps, oobavail); >>>>> > + >>>>> > + if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) && >>>>> > + !nand_match_ecc_req(chip, caps, oobavail)) >>>>> > + return 0; >>>>> > + >>>>> > + return nand_maximize_ecc(chip, caps, oobavail); >>>>> >>>>> I personally don't mind if nand_maximize_ecc() is called twice in >>>>> the function if it clarifies the logic. Maybe the following will be >>>>> more clear for the user? >>>>> >>>>> if (chip->ecc.size && chip->ecc.strength) >>>>> return nand_check_ecc_caps(chip, caps, oobavail); >>>>> >>>>> if (chip->ecc.options & NAND_ECC_MAXIMIZE) >>>>> return nand_maximize_ecc(chip, caps, oobavail); >>>>> >>>>> if (!nand_match_ecc_req(chip, caps, oobavail)) >>>>> return 0; >>>>> >>>>> return nand_maximize_ecc(chip, caps, oobavail); >>>> >>>> >>>> I personally don't mind, and it seems Masahiro wanted to keep the >>>> logic >>>> he had used in the denali driver. >>>> >>>>> >>>>> Also, I'm not sure we should just error out when >>>>> nand_check_ecc_caps() >>>>> fails. What about something more robust, like: >>>>> >>>>> int ret; >>>>> >>>>> if (chip->ecc.size && chip->ecc.strength) { >>>>> ret = nand_check_ecc_caps(chip, caps, oobavail); >>>>> if (ret) >>>>> goto maximize_ecc; >>>> >>>> >>>> Nope. When someone asked for a specific ECC config by passing the >>>> nand-ecc-xxx props we should apply it or return an erro if it's not >>>> supported. People passing those props should now what the ECC engine >>>> supports and pick one valid values. >>>> >>>>> >>>>> return 0; >>>>> } >>>>> >>>>> if (chip->ecc.options & NAND_ECC_MAXIMIZE) >>>>> goto maximize_ecc; >>>>> >>>>> ret = nand_match_ecc_req(chip, caps, oobavail); >>>>> if (ret) >>>>> goto maximize_ecc; >>>>> >>>>> return 0; >>>>> >>>>> maximize_ecc: >>>>> return nand_maximize_ecc(chip, caps, oobavail); >>>>> >>>> >>>> >>>> ______________________________________________________ >>>> Linux MTD discussion mailing list >>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/ >>> >>> >>> >>> >>> >>> >>> >>> This version looks good to me. >>> >>> If you want to check the error code more precisely, >>> how about something like follows? >>> >>> >>> >>> int nand_ecc_choose_conf(struct nand_chip *chip, >>> const struct nand_ecc_caps *caps, int >>> oobavail) >>> { >>> int ret; >>> >>> if (chip->ecc.size && chip->ecc.strength) >>> return nand_check_ecc_caps(chip, caps, oobavail); >>> >>> if (!(chip->ecc.options & NAND_ECC_MAXIMIZE)) { >>> ret = nand_match_ecc_req(chip, caps, oobavail); >>> if (ret != -ENOTSUPP) >>> return ret; >>> } >>> >>> return nand_maximize_ecc(chip, caps, oobavail); >>> } >>> >>> >>> Only the difference is the case >>> where nand_match_ecc_req() returns a different error code >>> than ENOTSUPP. >>> (Currently, this happens only when insane 'oobavail' is passed.) >>> >> >> We can do that but to me, it will make the helper function >> more complicated. Currently, nand_match_ecc_req is returning >> other than ENOTSUPP 'oobavail < 0' is passed. >> and again in nand_maximize_ecc, we will check for validity >> of oobavail so nothing wrong will happen in calling >> nand_maximize_ecc. > > > Right. When I added those three helpers, > I supposed they were independent APIs. > That is why I added the 'oobavail < 0' sanity check > in each of the three. > > > If you make them internal sub-helpers > (i.e. add 'static' instead of EXPORT_SYMBOL_GPL), > you can check 'oobavail < 0' > only in nand_ecc_choose_conf(). > >
I am not sure regarding making them static.
Currently, Denali NAND driver is only using these functions.
And Now, this nand_ecc_choose_conf will be help in all the cases.
For nand_check_ecc_caps: call nand_ecc_choose_conf with chip->ecc.size && chip->ecc.strength
For nand_maximize_ecc: call nand_ecc_choose_conf with NAND_ECC_MAXIMIZE
So making them static also seems ok which will be easy to maintain in future.
Thanks, Abhishek
> > > >> Anyway we put this under WARN_ON condition >> >> if (WARN_ON(oobavail < 0)) >> return -EINVAL; >> >> so if this is being triggered, then it should be mostly >> programming error. > > > Right. Moreover, > > WARN_ON(oobavail < 0 || oobavail > mtd->oobsize) > > > > This is programming error, that is why WARN_ON() is used to > make the log noisy. > > >> Thanks, >> Abhishek >> >>> >>> ENOTSUPP means 'required ECC setting is not supported'. >>> Other error code is more significant, so it is not a good reason >>> to fall back to miximization, IMHO. >> >> >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/
| |