2018-05-30 15:21 GMT+09:00 Abhishek Sahu <absahu@xxxxxxxxxxxxxx>:
> On 2018-05-30 05:58, Masahiro Yamada wrote:
>>
>> Hi.
>>
>> 2018-05-30 4:30 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxx>:
>>>
>>> On Sat, 26 May 2018 10:42:47 +0200
>>> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>>>
>>>> Hi Abhishek,
>>>>
>>>> On Fri, 25 May 2018 17:51:29 +0530, Abhishek Sahu
>>>> <absahu@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>>> > Signed-off-by: Abhishek Sahu <absahu@xxxxxxxxxxxxxx>
>>>> > ---
>>>> > * 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().
> 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/
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
