lkml.org 
[lkml]   [2018]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration
    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.

    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.

    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.

    \
     
     \ /
      Last update: 2018-05-30 08:22    [W:2.638 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site