On Fri, 26 Feb 2016 17:30:47 +0100
Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
> On 25 February 2016 at 21:09, Boris Brezillon
> <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
> > On Thu, 25 Feb 2016 20:56:36 +0100
> > Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
> >
> >> On 24 February 2016 at 14:46, Boris Brezillon
> >> <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
> >> > On Fri, 12 Feb 2016 19:11:23 +0100
> >> > Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
> >> >
> >> >> This allows specifying algorithm used for NAND ECC. There are two
> >> >> available values: "bch" and "hamming". It's important as having just
> >> >> ECC parameters (step, strength) isn't enough to determine algorithm,
> >> >> e.g. you can't distinct BCH-1 and Hamming.
> >> >>
> >> >> Signed-off-by: Rafał Miłecki <zajec5@xxxxxxxxx>
> >> >> ---
> >> >> Documentation/devicetree/bindings/mtd/nand.txt | 3 +++
> >> >> drivers/of/of_mtd.c | 33 ++++++++++++++++++++++++++
> >> >> include/linux/mtd/nand.h | 5 ++++
> >> >> include/linux/of_mtd.h | 6 +++++
> >> >> 4 files changed, 47 insertions(+)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
> >> >> index b53f92e..a2c2df5 100644
> >> >> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> >> >> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> >> >> @@ -3,6 +3,9 @@
> >> >> - nand-ecc-mode : String, operation mode of the NAND ecc mode.
> >> >> Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
> >> >> "soft_bch".
> >> >> +- nand-ecc-algo : string, algorithm of NAND ecc.
> >> >> + Supported values are: "bch", "hamming". The default one is
> >> >> + "bch".
> >> >
> >> > I like the idea of specifying which ECC algorithm should be used, and
> >> > this is IMO clearer than putting the information directly in
> >> > nand-ecc-mode (as is already done for the "soft" and "soft_bch" modes,
> >> > which are respectively encoding software hamming and software bch
> >> > implementations).
> >> >
> >> > But shouldn't we take this even further and add new DT properties
> >> > to encode the ECC layout (syndrome, oob_first), and another one to
> >> > specify the implementation type (software or hardware)?
> >> >
> >> > nand-ecc-layout = "nornal", "syndrome" or "oob_first"
> >> > nand-ecc-engine = "none", "soft" or "hw" ("on-die" ?)
> >> >
> >> > Note that it's not something I ask you to do right now, I just want to
> >> > bring the topic on the table.
> >>
> >> So far I was fine with whatever drvier/NAND core picked, didn't have
> >> any issue with it. If at some point we'll see a real need of
> >> specifying it manually as well, I guess we should do as you suggested.
> >>
> >
> > My point is that it's kind of weird to have the same information
> > duplicated in two different properties with possibly some conflicting
> > configurations, like:
> >
> > nand-ecc-mode = "soft_bch"; /* software BCH implementation... */
> > nand-ecc-algo = "hamming"; /* ... and here you choose Hamming */
> >
> > Of course this won't be a real problem until NAND core starts using
> > this property to decide which soft implementation should be used, but
> > providing a consistent binding is important IMO.
>
> Sorry, I didn't pay enough attention to NAND properties and missed
> your point. I was a bit surprised to see NAND_ECC_SOFT_BCH value and
> "soft_bch" as mapping. This is indeed a bit confusing.
>
> So I'm trying to find a proper way to split nand-ecc-mode. What you
> suggested isn't exactly clear to me. AFAIR both enums: SYNDROME and
> OOB_FIRST specify they way ECC info has to be accessed. For example
> when using NAND_ECC_HW_OOB_FIRST you have to read OOB and after that
> read data. I don't see how it's really related to the ECC layout (you
> suggested "oob_first" as possible value of nand-ecc-layout).
Because it's encoding where the ECC bytes are stored in a NAND page.
Maybe ecc-layout is not the correct name (how about nand-page-layout),
but neither ecc-mode is ;-).
>
> What about:
> 1) Adding new nand-ecc-algo as this patch does
> 2) Deprecating "soft_bch" value from nand-ecc-mode property
>
Sounds good to me.
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
