From: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> To: "Rafał Miłecki" <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" <linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Subject: Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT Date: Fri, 26 Feb 2016 17:55:06 +0100 [thread overview] Message-ID: <20160226175506.75d6c7b0@bbrezillon> (raw) In-Reply-To: <CACna6ryVuqSLv8WdzzGUQzpA-F35JxXoHoa2jcLzZeHaXtoexA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On Fri, 26 Feb 2016 17:30:47 +0100 Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > On 25 February 2016 at 21:09, Boris Brezillon > <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote: > > On Thu, 25 Feb 2016 20:56:36 +0100 > > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > >> On 24 February 2016 at 14:46, Boris Brezillon > >> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote: > >> > On Fri, 12 Feb 2016 19:11:23 +0100 > >> > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > >> >> --- > >> >> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Boris Brezillon <boris.brezillon@free-electrons.com> To: "Rafał Miłecki" <zajec5@gmail.com> Cc: Brian Norris <computersforpeace@gmail.com>, "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, Kamal Dasu <kdasu.kdev@gmail.com>, Hauke Mehrtens <hauke@hauke-m.de>, Rob Herring <robh+dt@kernel.org>, Grant Likely <grant.likely@linaro.org>, Frank Rowand <frowand.list@gmail.com> Subject: Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT Date: Fri, 26 Feb 2016 17:55:06 +0100 [thread overview] Message-ID: <20160226175506.75d6c7b0@bbrezillon> (raw) In-Reply-To: <CACna6ryVuqSLv8WdzzGUQzpA-F35JxXoHoa2jcLzZeHaXtoexA@mail.gmail.com> On Fri, 26 Feb 2016 17:30:47 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > On 25 February 2016 at 21:09, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Thu, 25 Feb 2016 20:56:36 +0100 > > Rafał Miłecki <zajec5@gmail.com> wrote: > > > >> On 24 February 2016 at 14:46, Boris Brezillon > >> <boris.brezillon@free-electrons.com> wrote: > >> > On Fri, 12 Feb 2016 19:11:23 +0100 > >> > Rafał Miłecki <zajec5@gmail.com> 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@gmail.com> > >> >> --- > >> >> 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
next prev parent reply other threads:[~2016-02-26 16:55 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-12 18:11 [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT Rafał Miłecki 2016-02-12 18:11 ` Rafał Miłecki 2016-02-15 21:28 ` Kamal Dasu [not found] ` <1455300685-27009-1-git-send-email-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-02-12 18:11 ` [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm Rafał Miłecki 2016-02-12 18:11 ` Rafał Miłecki [not found] ` <1455300685-27009-2-git-send-email-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-02-15 21:31 ` Kamal Dasu 2016-02-15 21:31 ` Kamal Dasu 2016-04-01 16:02 ` Brian Norris 2016-04-01 16:02 ` Brian Norris 2016-04-01 16:07 ` Brian Norris 2016-04-01 16:07 ` Brian Norris [not found] ` <20160401160722.GC117117-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 2016-04-01 19:32 ` Rafał Miłecki 2016-04-01 19:32 ` Rafał Miłecki [not found] ` <CACna6rz6mYSxAr1oSjcLc__sSKM1XdUf-cGOFanGvuij0nNC5Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-04-01 20:07 ` Brian Norris 2016-04-01 20:07 ` Brian Norris 2016-04-01 21:23 ` Rafał Miłecki 2016-04-01 21:23 ` Rafał Miłecki 2016-02-12 18:11 ` [PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm Rafał Miłecki 2016-02-12 18:11 ` Rafał Miłecki [not found] ` <1455300685-27009-3-git-send-email-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-02-15 21:30 ` Kamal Dasu 2016-02-15 21:30 ` Kamal Dasu 2016-04-01 16:07 ` Brian Norris 2016-04-01 16:07 ` Brian Norris 2016-02-15 21:43 ` [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT Kamal Dasu 2016-02-15 21:43 ` Kamal Dasu 2016-02-22 2:54 ` Rob Herring 2016-02-22 2:54 ` Rob Herring 2016-02-24 13:46 ` Boris Brezillon 2016-02-24 13:46 ` Boris Brezillon 2016-02-25 19:56 ` Rafał Miłecki 2016-02-25 19:56 ` Rafał Miłecki [not found] ` <CACna6rxemvBOu9fy9KP0327TsqonO7dFY5EsQjf1k9Ln6hX8cw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-25 20:09 ` Boris Brezillon 2016-02-25 20:09 ` Boris Brezillon 2016-02-26 16:30 ` Rafał Miłecki 2016-02-26 16:30 ` Rafał Miłecki [not found] ` <CACna6ryVuqSLv8WdzzGUQzpA-F35JxXoHoa2jcLzZeHaXtoexA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-26 16:55 ` Boris Brezillon [this message] 2016-02-26 16:55 ` Boris Brezillon 2016-02-26 21:24 ` Rafał Miłecki 2016-02-26 21:24 ` Rafał Miłecki [not found] ` <CACna6rzRROfYwagsBGo5b8d_bydUBrJ=9RvGZSt1G85xP=gKaw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-26 21:28 ` Boris Brezillon 2016-02-26 21:28 ` Boris Brezillon 2016-04-01 16:01 ` Brian Norris 2016-04-01 16:01 ` Brian Norris
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160226175506.75d6c7b0@bbrezillon \ --to=boris.brezillon-wi1+55scjutkeb57/3fjtnbpr1lh4cv8@public.gmane.org \ --cc=computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ --cc=hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org \ --cc=kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.