Message ID | 1455300685-27009-1-git-send-email-zajec5@gmail.com |
---|---|
State | Not Applicable, archived |
Headers | show |
On Fri, Feb 12, 2016 at 1:11 PM, Rafał Miłecki <zajec5@gmail.com> wrote: > So far we were treating ECC strength 1 as Hamming algorithm. It didn't > supporting some less common devices with BCH-1 (e.g. D-Link DIR-885L). > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> Reviewed-by: Kamal Dasu <kdasu.kdev@gmail.com> Thanks Rafal > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 844fc07..b8055da 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -1842,7 +1842,7 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > > switch (chip->ecc.size) { > case 512: > - if (chip->ecc.strength == 1) /* Hamming */ > + if (chip->ecc.algo == NAND_ECC_HAMMING) > cfg->ecc_level = 15; > else > cfg->ecc_level = chip->ecc.strength; > -- > 1.8.4.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 1:11 PM, Rafał Miłecki <zajec5@gmail.com> wrote: > > This will allow drivers handle ECC properly. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> Reviewed-by: Kamal Dasu <kdasu.kdev@gmail.com> Thanks Rafal > > --- > drivers/mtd/nand/nand_base.c | 6 +++++- > include/linux/mtd/nand.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index f2c8ff3..ef977f3 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -3979,7 +3979,7 @@ ident_done: > static int nand_dt_init(struct nand_chip *chip) > { > struct device_node *dn = nand_get_flash_node(chip); > - int ecc_mode, ecc_strength, ecc_step; > + int ecc_mode, ecc_algo, ecc_strength, ecc_step; > > if (!dn) > return 0; > @@ -3991,6 +3991,7 @@ static int nand_dt_init(struct nand_chip *chip) > chip->bbt_options |= NAND_BBT_USE_FLASH; > > ecc_mode = of_get_nand_ecc_mode(dn); > + ecc_algo = of_get_nand_ecc_algo(dn); > ecc_strength = of_get_nand_ecc_strength(dn); > ecc_step = of_get_nand_ecc_step_size(dn); > > @@ -4003,6 +4004,9 @@ static int nand_dt_init(struct nand_chip *chip) > if (ecc_mode >= 0) > chip->ecc.mode = ecc_mode; > > + if (ecc_algo >= 0) > + chip->ecc.algo = ecc_algo; > + > if (ecc_strength >= 0) > chip->ecc.strength = ecc_strength; > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 25854d2..8deca1b 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -513,6 +513,7 @@ struct nand_hw_control { > */ > struct nand_ecc_ctrl { > nand_ecc_modes_t mode; > + enum nand_ecc_algo algo; > int steps; > int size; > int bytes; > -- > 1.8.4.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 1:11 PM, 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> Reviewed-by: Kamal Dasu <kdasu.kdev@gmail.com> Thanks Rafal > --- > 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". > - nand-bus-width : 8 or 16 bus width if not present 8 > - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false > > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index b7361ed..f2a6630 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -50,6 +50,39 @@ int of_get_nand_ecc_mode(struct device_node *np) > EXPORT_SYMBOL_GPL(of_get_nand_ecc_mode); > > /** > + * It maps 'enum nand_ecc_algo' found in include/linux/mtd/nand.h into the > + * device tree binding of 'nand-ecc-algo'. > + */ > +static const char * const nand_ecc_algos[] = { > + [NAND_ECC_BCH] = "bch", > + [NAND_ECC_HAMMING] = "hamming", > +}; > + > +/** > + * of_get_nand_ecc_algo - Get nand ecc algorithm for given device_node > + * @np: Pointer to the given device_node > + * > + * The function gets ecc algorithm string from property 'nand-ecc-algo' and > + * returns its index in nand_ecc_algos table, or errno in error case. > + */ > +int of_get_nand_ecc_algo(struct device_node *np) > +{ > + const char *pm; > + int err, i; > + > + err = of_property_read_string(np, "nand-ecc-algo", &pm); > + if (err < 0) > + return err; > + > + for (i = 0; i < ARRAY_SIZE(nand_ecc_algos); i++) > + if (!strcasecmp(pm, nand_ecc_algos[i])) > + return i; > + > + return -ENODEV; > +} > +EXPORT_SYMBOL_GPL(of_get_nand_ecc_algo); > + > +/** > * of_get_nand_ecc_step_size - Get ECC step size associated to > * the required ECC strength (see below). > * @np: Pointer to the given device_node > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 7604f4b..25854d2 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -119,6 +119,11 @@ typedef enum { > NAND_ECC_SOFT_BCH, > } nand_ecc_modes_t; > > +enum nand_ecc_algo { > + NAND_ECC_BCH, > + NAND_ECC_HAMMING, > +}; > + > /* > * Constants for Hardware ECC > */ > diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h > index e266caa..0f6aca5 100644 > --- a/include/linux/of_mtd.h > +++ b/include/linux/of_mtd.h > @@ -13,6 +13,7 @@ > > #include <linux/of.h> > int of_get_nand_ecc_mode(struct device_node *np); > +int of_get_nand_ecc_algo(struct device_node *np); > int of_get_nand_ecc_step_size(struct device_node *np); > int of_get_nand_ecc_strength(struct device_node *np); > int of_get_nand_bus_width(struct device_node *np); > @@ -25,6 +26,11 @@ static inline int of_get_nand_ecc_mode(struct device_node *np) > return -ENOSYS; > } > > +static inline int of_get_nand_ecc_algo(struct device_node *np) > +{ > + return -ENOSYS; > +} > + > static inline int of_get_nand_ecc_step_size(struct device_node *np) > { > return -ENOSYS; > -- > 1.8.4.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 07:11:23PM +0100, Rafał Miłecki 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 +++ Acked-by: Rob Herring <robh@kernel.org> > drivers/of/of_mtd.c | 33 ++++++++++++++++++++++++++ > include/linux/mtd/nand.h | 5 ++++ > include/linux/of_mtd.h | 6 +++++ > 4 files changed, 47 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafal, 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. Best Regards, Boris
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.
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.
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). What about: 1) Adding new nand-ecc-algo as this patch does 2) Deprecating "soft_bch" value from nand-ecc-mode property
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.
On 26 February 2016 at 17:55, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Fri, 26 Feb 2016 17:30:47 +0100 > Rafał Miłecki <zajec5@gmail.com> wrote: >> 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. I have problems understanding "soft" values which maps to the NAND_ECC_SOFT. What exactly algorithm is that? It uses functions from drivers/mtd/nand/nand_ecc.c but this file isn't really helpful. I was only able to find "3-byte ECC" phase. Kconfig config MTD_NAND_ECC also doesn't have any description. I also tried looking at Documentation/mtd/nand_ecc.txt but still no luck. Does this algorithm has any name?
On Fri, 26 Feb 2016 22:24:21 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > On 26 February 2016 at 17:55, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Fri, 26 Feb 2016 17:30:47 +0100 > > Rafał Miłecki <zajec5@gmail.com> wrote: > >> 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. > > I have problems understanding "soft" values which maps to the > NAND_ECC_SOFT. What exactly algorithm is that? It uses functions from > drivers/mtd/nand/nand_ecc.c but this file isn't really helpful. I was > only able to find "3-byte ECC" phase. Kconfig config MTD_NAND_ECC also > doesn't have any description. I also tried looking at > Documentation/mtd/nand_ecc.txt but still no luck. > > Does this algorithm has any name? > AFAIR, nand_ecc.c is implementing Hamming ECC.
On Fri, Feb 12, 2016 at 07:11:23PM +0100, Rafał Miłecki 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> Acked-by: Brian Norris <computersforpeace@gmai.com> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 07:11:24PM +0100, Rafał Miłecki wrote: > This will allow drivers handle ECC properly. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> Acked-by: Brian Norris <computersforpeace@gmai.com> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 07:11:24PM +0100, Rafał Miłecki wrote: > This will allow drivers handle ECC properly. > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > drivers/mtd/nand/nand_base.c | 6 +++++- > include/linux/mtd/nand.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index f2c8ff3..ef977f3 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -3979,7 +3979,7 @@ ident_done: > static int nand_dt_init(struct nand_chip *chip) > { > struct device_node *dn = nand_get_flash_node(chip); > - int ecc_mode, ecc_strength, ecc_step; > + int ecc_mode, ecc_algo, ecc_strength, ecc_step; > > if (!dn) > return 0; > @@ -3991,6 +3991,7 @@ static int nand_dt_init(struct nand_chip *chip) > chip->bbt_options |= NAND_BBT_USE_FLASH; > > ecc_mode = of_get_nand_ecc_mode(dn); > + ecc_algo = of_get_nand_ecc_algo(dn); > ecc_strength = of_get_nand_ecc_strength(dn); > ecc_step = of_get_nand_ecc_step_size(dn); > > @@ -4003,6 +4004,9 @@ static int nand_dt_init(struct nand_chip *chip) > if (ecc_mode >= 0) > chip->ecc.mode = ecc_mode; > > + if (ecc_algo >= 0) > + chip->ecc.algo = ecc_algo; > + While this might appear to handle the absence of the nand-ecc-algo property correctly, this isn't safe. This means that any existing DT without the property will get treated as Hamming ECC. Perhaps we need the enum to have a 0 value of 'NAND_ECC_ALGO_UNKNOWN' or something like that? Brian > if (ecc_strength >= 0) > chip->ecc.strength = ecc_strength; > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 25854d2..8deca1b 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -513,6 +513,7 @@ struct nand_hw_control { > */ > struct nand_ecc_ctrl { > nand_ecc_modes_t mode; > + enum nand_ecc_algo algo; > int steps; > int size; > int bytes; > -- > 1.8.4.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 12, 2016 at 07:11:25PM +0100, Rafał Miłecki wrote: > So far we were treating ECC strength 1 as Hamming algorithm. It didn't > supporting some less common devices with BCH-1 (e.g. D-Link DIR-885L). > > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 844fc07..b8055da 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -1842,7 +1842,7 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > > switch (chip->ecc.size) { > case 512: > - if (chip->ecc.strength == 1) /* Hamming */ > + if (chip->ecc.algo == NAND_ECC_HAMMING) It's probably best we do a little more than this. Right now, this allows someone to specify: nand-ecc-strength = <20>; nand-ecc-stepsize = <512>; nand-ecc-algo = "hamming"; nand-ecc-mode = "hw"; And they'll end up with 1-bit hamming ECC, except nand_base will still think we have 20-bit correction. I think we need to add a check in this driver to be sure we haven't selected >1-bit correction with hamming ECC. Brian > cfg->ecc_level = 15; > else > cfg->ecc_level = chip->ecc.strength; > -- > 1.8.4.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 April 2016 at 18:07, Brian Norris <computersforpeace@gmail.com> wrote: > On Fri, Feb 12, 2016 at 07:11:24PM +0100, Rafał Miłecki wrote: >> This will allow drivers handle ECC properly. >> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >> --- >> drivers/mtd/nand/nand_base.c | 6 +++++- >> include/linux/mtd/nand.h | 1 + >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index f2c8ff3..ef977f3 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -3979,7 +3979,7 @@ ident_done: >> static int nand_dt_init(struct nand_chip *chip) >> { >> struct device_node *dn = nand_get_flash_node(chip); >> - int ecc_mode, ecc_strength, ecc_step; >> + int ecc_mode, ecc_algo, ecc_strength, ecc_step; >> >> if (!dn) >> return 0; >> @@ -3991,6 +3991,7 @@ static int nand_dt_init(struct nand_chip *chip) >> chip->bbt_options |= NAND_BBT_USE_FLASH; >> >> ecc_mode = of_get_nand_ecc_mode(dn); >> + ecc_algo = of_get_nand_ecc_algo(dn); >> ecc_strength = of_get_nand_ecc_strength(dn); >> ecc_step = of_get_nand_ecc_step_size(dn); >> >> @@ -4003,6 +4004,9 @@ static int nand_dt_init(struct nand_chip *chip) >> if (ecc_mode >= 0) >> chip->ecc.mode = ecc_mode; >> >> + if (ecc_algo >= 0) >> + chip->ecc.algo = ecc_algo; >> + > > While this might appear to handle the absence of the nand-ecc-algo > property correctly, this isn't safe. This means that any existing DT > without the property will get treated as Hamming ECC. Perhaps we need > the enum to have a 0 value of 'NAND_ECC_ALGO_UNKNOWN' or something like > that? You're commenting on an old series. If you take a look at: https://patchwork.ozlabs.org/patch/601180/ that also got applied to the: https://github.com/linux-nand/linux/commits/nand/next repo, you'll see we have NAND_ECC_UNKNOWN there. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 01, 2016 at 09:32:40PM +0200, Rafał Miłecki wrote: > On 1 April 2016 at 18:07, Brian Norris <computersforpeace@gmail.com> wrote: > > On Fri, Feb 12, 2016 at 07:11:24PM +0100, Rafał Miłecki wrote: > >> This will allow drivers handle ECC properly. > >> > >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > >> --- > >> drivers/mtd/nand/nand_base.c | 6 +++++- > >> include/linux/mtd/nand.h | 1 + > >> 2 files changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > >> index f2c8ff3..ef977f3 100644 > >> --- a/drivers/mtd/nand/nand_base.c > >> +++ b/drivers/mtd/nand/nand_base.c > >> @@ -3979,7 +3979,7 @@ ident_done: > >> static int nand_dt_init(struct nand_chip *chip) > >> { > >> struct device_node *dn = nand_get_flash_node(chip); > >> - int ecc_mode, ecc_strength, ecc_step; > >> + int ecc_mode, ecc_algo, ecc_strength, ecc_step; > >> > >> if (!dn) > >> return 0; > >> @@ -3991,6 +3991,7 @@ static int nand_dt_init(struct nand_chip *chip) > >> chip->bbt_options |= NAND_BBT_USE_FLASH; > >> > >> ecc_mode = of_get_nand_ecc_mode(dn); > >> + ecc_algo = of_get_nand_ecc_algo(dn); > >> ecc_strength = of_get_nand_ecc_strength(dn); > >> ecc_step = of_get_nand_ecc_step_size(dn); > >> > >> @@ -4003,6 +4004,9 @@ static int nand_dt_init(struct nand_chip *chip) > >> if (ecc_mode >= 0) > >> chip->ecc.mode = ecc_mode; > >> > >> + if (ecc_algo >= 0) > >> + chip->ecc.algo = ecc_algo; > >> + > > > > While this might appear to handle the absence of the nand-ecc-algo > > property correctly, this isn't safe. This means that any existing DT > > without the property will get treated as Hamming ECC. Perhaps we need > > the enum to have a 0 value of 'NAND_ECC_ALGO_UNKNOWN' or something like > > that? > > You're commenting on an old series. If you take a look at: > https://patchwork.ozlabs.org/patch/601180/ > that also got applied to the: > https://github.com/linux-nand/linux/commits/nand/next > repo, you'll see we have NAND_ECC_UNKNOWN there. Ah, I guess I missed that because I was searching for the patch to brcmnand (i.e., fixing the original problem you saw). AFAICT, this v2 series doesn't actually resolve your issue with brcmnand, no? Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1 April 2016 at 22:07, Brian Norris <computersforpeace@gmail.com> wrote: > On Fri, Apr 01, 2016 at 09:32:40PM +0200, Rafał Miłecki wrote: >> On 1 April 2016 at 18:07, Brian Norris <computersforpeace@gmail.com> wrote: >> > On Fri, Feb 12, 2016 at 07:11:24PM +0100, Rafał Miłecki wrote: >> >> This will allow drivers handle ECC properly. >> >> >> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >> >> --- >> >> drivers/mtd/nand/nand_base.c | 6 +++++- >> >> include/linux/mtd/nand.h | 1 + >> >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> >> index f2c8ff3..ef977f3 100644 >> >> --- a/drivers/mtd/nand/nand_base.c >> >> +++ b/drivers/mtd/nand/nand_base.c >> >> @@ -3979,7 +3979,7 @@ ident_done: >> >> static int nand_dt_init(struct nand_chip *chip) >> >> { >> >> struct device_node *dn = nand_get_flash_node(chip); >> >> - int ecc_mode, ecc_strength, ecc_step; >> >> + int ecc_mode, ecc_algo, ecc_strength, ecc_step; >> >> >> >> if (!dn) >> >> return 0; >> >> @@ -3991,6 +3991,7 @@ static int nand_dt_init(struct nand_chip *chip) >> >> chip->bbt_options |= NAND_BBT_USE_FLASH; >> >> >> >> ecc_mode = of_get_nand_ecc_mode(dn); >> >> + ecc_algo = of_get_nand_ecc_algo(dn); >> >> ecc_strength = of_get_nand_ecc_strength(dn); >> >> ecc_step = of_get_nand_ecc_step_size(dn); >> >> >> >> @@ -4003,6 +4004,9 @@ static int nand_dt_init(struct nand_chip *chip) >> >> if (ecc_mode >= 0) >> >> chip->ecc.mode = ecc_mode; >> >> >> >> + if (ecc_algo >= 0) >> >> + chip->ecc.algo = ecc_algo; >> >> + >> > >> > While this might appear to handle the absence of the nand-ecc-algo >> > property correctly, this isn't safe. This means that any existing DT >> > without the property will get treated as Hamming ECC. Perhaps we need >> > the enum to have a 0 value of 'NAND_ECC_ALGO_UNKNOWN' or something like >> > that? >> >> You're commenting on an old series. If you take a look at: >> https://patchwork.ozlabs.org/patch/601180/ >> that also got applied to the: >> https://github.com/linux-nand/linux/commits/nand/next >> repo, you'll see we have NAND_ECC_UNKNOWN there. > > Ah, I guess I missed that because I was searching for the patch to > brcmnand (i.e., fixing the original problem you saw). AFAICT, this v2 > series doesn't actually resolve your issue with brcmnand, no? That's correct. I'm planning to clean NAND subsystem first, then patch brcmnand.
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". - nand-bus-width : 8 or 16 bus width if not present 8 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c index b7361ed..f2a6630 100644 --- a/drivers/of/of_mtd.c +++ b/drivers/of/of_mtd.c @@ -50,6 +50,39 @@ int of_get_nand_ecc_mode(struct device_node *np) EXPORT_SYMBOL_GPL(of_get_nand_ecc_mode); /** + * It maps 'enum nand_ecc_algo' found in include/linux/mtd/nand.h into the + * device tree binding of 'nand-ecc-algo'. + */ +static const char * const nand_ecc_algos[] = { + [NAND_ECC_BCH] = "bch", + [NAND_ECC_HAMMING] = "hamming", +}; + +/** + * of_get_nand_ecc_algo - Get nand ecc algorithm for given device_node + * @np: Pointer to the given device_node + * + * The function gets ecc algorithm string from property 'nand-ecc-algo' and + * returns its index in nand_ecc_algos table, or errno in error case. + */ +int of_get_nand_ecc_algo(struct device_node *np) +{ + const char *pm; + int err, i; + + err = of_property_read_string(np, "nand-ecc-algo", &pm); + if (err < 0) + return err; + + for (i = 0; i < ARRAY_SIZE(nand_ecc_algos); i++) + if (!strcasecmp(pm, nand_ecc_algos[i])) + return i; + + return -ENODEV; +} +EXPORT_SYMBOL_GPL(of_get_nand_ecc_algo); + +/** * of_get_nand_ecc_step_size - Get ECC step size associated to * the required ECC strength (see below). * @np: Pointer to the given device_node diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 7604f4b..25854d2 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -119,6 +119,11 @@ typedef enum { NAND_ECC_SOFT_BCH, } nand_ecc_modes_t; +enum nand_ecc_algo { + NAND_ECC_BCH, + NAND_ECC_HAMMING, +}; + /* * Constants for Hardware ECC */ diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h index e266caa..0f6aca5 100644 --- a/include/linux/of_mtd.h +++ b/include/linux/of_mtd.h @@ -13,6 +13,7 @@ #include <linux/of.h> int of_get_nand_ecc_mode(struct device_node *np); +int of_get_nand_ecc_algo(struct device_node *np); int of_get_nand_ecc_step_size(struct device_node *np); int of_get_nand_ecc_strength(struct device_node *np); int of_get_nand_bus_width(struct device_node *np); @@ -25,6 +26,11 @@ static inline int of_get_nand_ecc_mode(struct device_node *np) return -ENOSYS; } +static inline int of_get_nand_ecc_algo(struct device_node *np) +{ + return -ENOSYS; +} + static inline int of_get_nand_ecc_step_size(struct device_node *np) { return -ENOSYS;
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(+)