Message-ID: <CAK7LNATgPafev_ZCjzi5jPS3Wg1bq_V9=_VUnrpcUqbFAGJpJg@mail.gmail.com>
Date:   Sun, 23 Apr 2017 00:00:57 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:     linux-mtd@...ts.infradead.org, Enrico Jorns <ejo@...gutronix.de>,
        Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Graham Moore <grmoore@...nsource.altera.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Chuanxiao Dong <chuanxiao.dong@...el.com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        devicetree@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Brian Norris <computersforpeace@...il.com>,
        Richard Weinberger <richard@....at>,
        Cyrille Pitchen <cyrille.pitchen@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v3 14/37] mtd: nand: denali: support "nand-ecc-strength"
 DT property

Hi Boris,


2017-04-14 17:19 GMT+09:00 Boris Brezillon <boris.brezillon@...e-electrons.com>:
> On Fri, 14 Apr 2017 16:57:23 +0900
> Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
>
>> Hi Boris,
>>
>>
>> 2017-04-11 16:56 GMT+09:00 Boris Brezillon <boris.brezillon@...e-electrons.com>:
>> > Hi Masahiro,
>> >
>> > On Tue, 11 Apr 2017 15:19:21 +0900
>> > Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >>
>> >>
>> >> 2017-04-10 1:33 GMT+09:00 Boris Brezillon <boris.brezillon@...e-electrons.com>:
>> >> > On Mon, 3 Apr 2017 12:16:34 +0900
>> >> > Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
>> >> >
>> >> >> Hi Boris,
>> >> >>
>> >> >>
>> >> >>
>> >> >> 2017-03-31 18:46 GMT+09:00 Boris Brezillon <boris.brezillon@...e-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?



-- 
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists