2016-04-22 11:23:28

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 1/3] mtd: nand: add support for "nand-ecc-algo" DT property

So far it was only possible to specify ECC algorithm using "soft" and
"soft_bch" values of nand-ecc-mode prop. There wasn't a way to specify
it for a hardware ECC mode.

Now that we have independent field in NAND subsystem for storing info
about ECC algorithm we may also add support for this new DT property.

Signed-off-by: Rafał Miłecki <[email protected]>
---
Documentation/devicetree/bindings/mtd/nand.txt | 2 ++
drivers/mtd/nand/nand_base.c | 20 +++++++++++++-------
2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
index a17662b..5ac4ab7 100644
--- a/Documentation/devicetree/bindings/mtd/nand.txt
+++ b/Documentation/devicetree/bindings/mtd/nand.txt
@@ -22,6 +22,8 @@ Optional NAND chip properties:
- 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: "hamming", "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/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7bc37b4..a5417a0 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4003,17 +4003,23 @@ static int of_get_nand_ecc_mode(struct device_node *np)
return -ENODEV;
}

+static const char * const nand_ecc_algos[] = {
+ [NAND_ECC_HAMMING] = "hamming",
+ [NAND_ECC_BCH] = "bch",
+};
+
static int of_get_nand_ecc_algo(struct device_node *np)
{
const char *pm;
- int err;
+ int err, i;

- /*
- * TODO: Read ECC algo OF property and map it to enum nand_ecc_algo.
- * It's not implemented yet as currently NAND subsystem ignores
- * algorithm explicitly set this way. Once it's handled we should
- * document & support new property.
- */
+ err = of_property_read_string(np, "nand-ecc-algo", &pm);
+ if (!err) {
+ for (i = 0; i < ARRAY_SIZE(nand_ecc_algos); i++)
+ if (!strcasecmp(pm, nand_ecc_algos[i]))
+ return i;
+ return -ENODEV;
+ }

/*
* For backward compatibility we also read "nand-ecc-mode" checking
--
1.8.4.5


2016-04-22 11:23:32

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 2/3] Documentation: devicetree: deprecate "soft_bch" nand-ecc-mode value

Now that we support nand-ecc-algo property it should be used together
with "soft" to specify software BCH ECC.

Signed-off-by: Rafał Miłecki <[email protected]>
---
Documentation/devicetree/bindings/mtd/nand.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
index 5ac4ab7..68342ea 100644
--- a/Documentation/devicetree/bindings/mtd/nand.txt
+++ b/Documentation/devicetree/bindings/mtd/nand.txt
@@ -20,8 +20,10 @@ Optional NAND controller properties
Optional NAND chip properties:

- nand-ecc-mode : String, operation mode of the NAND ecc mode.
- Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
- "soft_bch".
+ Supported values are: "none", "soft", "hw", "hw_syndrome",
+ "hw_oob_first".
+ Deprecated values:
+ "soft_bch": use "soft" and nand-ecc-algo instead
- nand-ecc-algo: string, algorithm of NAND ECC.
Supported values are: "hamming", "bch".
- nand-bus-width : 8 or 16 bus width if not present 8
--
1.8.4.5

2016-04-22 11:25:33

by Rafał Miłecki

[permalink] [raw]
Subject: [PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem

It's more reliable than guessing based on ECC strength. It allows using
NAND on devices with BCH-1 (e.g. D-Link DIR-885L).

Signed-off-by: Rafał Miłecki <[email protected]>
---
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 c3331ff..dcb22dc 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -1927,7 +1927,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

2016-04-25 10:11:59

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: nand: add support for "nand-ecc-algo" DT property

On Fri, 22 Apr 2016 13:23:13 +0200
Rafał Miłecki <[email protected]> wrote:

> So far it was only possible to specify ECC algorithm using "soft" and
> "soft_bch" values of nand-ecc-mode prop. There wasn't a way to specify
> it for a hardware ECC mode.
>
> Now that we have independent field in NAND subsystem for storing info
> about ECC algorithm we may also add support for this new DT property.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/nand.txt | 2 ++
> drivers/mtd/nand/nand_base.c | 20 +++++++++++++-------
> 2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
> index a17662b..5ac4ab7 100644
> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> @@ -22,6 +22,8 @@ Optional NAND chip properties:
> - 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: "hamming", "bch".

Rob, any objection to this binding change (and the one in patch 3).
Please note that everything is backward compatible.

Thanks,

Boris

> - 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/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 7bc37b4..a5417a0 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4003,17 +4003,23 @@ static int of_get_nand_ecc_mode(struct device_node *np)
> return -ENODEV;
> }
>
> +static const char * const nand_ecc_algos[] = {
> + [NAND_ECC_HAMMING] = "hamming",
> + [NAND_ECC_BCH] = "bch",
> +};
> +
> static int of_get_nand_ecc_algo(struct device_node *np)
> {
> const char *pm;
> - int err;
> + int err, i;
>
> - /*
> - * TODO: Read ECC algo OF property and map it to enum nand_ecc_algo.
> - * It's not implemented yet as currently NAND subsystem ignores
> - * algorithm explicitly set this way. Once it's handled we should
> - * document & support new property.
> - */
> + err = of_property_read_string(np, "nand-ecc-algo", &pm);
> + if (!err) {
> + for (i = 0; i < ARRAY_SIZE(nand_ecc_algos); i++)
> + if (!strcasecmp(pm, nand_ecc_algos[i]))
> + return i;
> + return -ENODEV;
> + }
>
> /*
> * For backward compatibility we also read "nand-ecc-mode" checking



--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2016-04-25 12:39:16

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: nand: add support for "nand-ecc-algo" DT property

On Fri, Apr 22, 2016 at 01:23:13PM +0200, Rafał Miłecki wrote:
> So far it was only possible to specify ECC algorithm using "soft" and
> "soft_bch" values of nand-ecc-mode prop. There wasn't a way to specify
> it for a hardware ECC mode.
>
> Now that we have independent field in NAND subsystem for storing info
> about ECC algorithm we may also add support for this new DT property.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/nand.txt | 2 ++
> drivers/mtd/nand/nand_base.c | 20 +++++++++++++-------
> 2 files changed, 15 insertions(+), 7 deletions(-)

Acked-by: Rob Herring <[email protected]>

2016-04-25 12:40:26

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 2/3] Documentation: devicetree: deprecate "soft_bch" nand-ecc-mode value

On Fri, Apr 22, 2016 at 01:23:14PM +0200, Rafał Miłecki wrote:
> Now that we support nand-ecc-algo property it should be used together
> with "soft" to specify software BCH ECC.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/nand.txt | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)

Acked-by: Rob Herring <[email protected]>

2016-04-25 15:05:24

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem

On Fri, 22 Apr 2016 13:23:15 +0200
Rafał Miłecki <[email protected]> wrote:

> It's more reliable than guessing based on ECC strength. It allows using
> NAND on devices with BCH-1 (e.g. D-Link DIR-885L).

Brian, Kamal, could you add your Ack on this patch.

>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> 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 c3331ff..dcb22dc 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1927,7 +1927,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;



--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2016-04-26 05:54:13

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem

On Fri, Apr 22, 2016 at 01:23:15PM +0200, Rafał Miłecki wrote:
> It's more reliable than guessing based on ECC strength. It allows using
> NAND on devices with BCH-1 (e.g. D-Link DIR-885L).
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> 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 c3331ff..dcb22dc 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1927,7 +1927,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)

This doesn't handle most of the problems I noted on the early version of
this series. (But thank you for following through on the algorithm
selection refactoring!)

Particularly, this change will
(a) break any existing DTs which used to have 'nand-ecc-size = <1>', and
would assume this gets Hamming ECC; and
(b) allows DTs to specify Hamming ECC for unsupported modes, like 1024B
sectors, or ecc_level != 1. None of these are supported in HW.

> cfg->ecc_level = 15;
> else
> cfg->ecc_level = chip->ecc.strength;

Something like the following probably works better (not tested):

---8<---

From: Brian Norris <[email protected]>
Date: Mon, 25 Apr 2016 20:48:02 -0700
Subject: [PATCH] mtd: brcmnand: respect ECC algorithm set by the NAND
subsystem

This is more obvious than guessing based on ECC strength. It allows
using NAND on devices with BCH-1 (e.g. D-Link DIR-885L).

This maintains DT backward compatibility by defaulting to Hamming if a
1-bit ECC algorithm is specified without a corresponding algorithm
selection. i.e., to use BCH-1, you must specify:

nand-ecc-strength = <1>;
nand-ecc-step-size = <512>;
nand-ecc-algo = "bch";

Also adds a check to ensure we haven't allowed someone to get by with SW
ECC. If we want to support SW ECC, we need to refactor some other pieces
of this driver.

Signed-off-by: Brian Norris <[email protected]>
---
drivers/mtd/nand/brcmnand/brcmnand.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index c3331ffcaffd..b76ad7c0144f 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -1925,9 +1925,31 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
cfg->col_adr_bytes = 2;
cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);

+ if (chip->ecc.mode != NAND_ECC_HW) {
+ dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
+ chip->ecc.mode);
+ return -EINVAL;
+ }
+
+ if (chip->ecc.algo == NAND_ECC_UNKNOWN) {
+ if (chip->ecc.strength == 1 && chip->ecc.size == 512)
+ /* Default to Hamming for 1-bit ECC, if unspecified */
+ chip->ecc.algo = NAND_ECC_HAMMING;
+ else
+ /* Otherwise, BCH */
+ chip->ecc.algo = NAND_ECC_BCH;
+ }
+
+ if (chip->ecc.algo == NAND_ECC_HAMMING && (chip->ecc.strength != 1 ||
+ chip->ecc.size != 512)) {
+ dev_err(ctrl->dev, "invalid Hamming params: %d bits per %d bytes\n",
+ chip->ecc.strength, chip->ecc.size);
+ return -EINVAL;
+ }
+
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;
--
2.8.0.rc3.226.g39d4020

2016-04-26 07:35:14

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 2/3] Documentation: devicetree: deprecate "soft_bch" nand-ecc-mode value

On Fri, 22 Apr 2016 13:23:14 +0200
Rafał Miłecki <[email protected]> wrote:

> Now that we support nand-ecc-algo property it should be used together
> with "soft" to specify software BCH ECC.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/nand.txt | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)

Applied, thanks.

Boris
--
Boris Brezillon, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2016-04-26 07:35:04

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 1/3] mtd: nand: add support for "nand-ecc-algo" DT property

On Fri, 22 Apr 2016 13:23:13 +0200
Rafał Miłecki <[email protected]> wrote:

> So far it was only possible to specify ECC algorithm using "soft" and
> "soft_bch" values of nand-ecc-mode prop. There wasn't a way to specify
> it for a hardware ECC mode.
>
> Now that we have independent field in NAND subsystem for storing info
> about ECC algorithm we may also add support for this new DT property.
>
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/nand.txt | 2 ++
> drivers/mtd/nand/nand_base.c | 20 +++++++++++++-------
> 2 files changed, 15 insertions(+), 7 deletions(-)

Applied, thanks.

Boris
--
Boris Brezillon, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

2016-04-26 07:37:50

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem

Hi Brian,

On Mon, 25 Apr 2016 22:53:55 -0700
Brian Norris <[email protected]> wrote:

> On Fri, Apr 22, 2016 at 01:23:15PM +0200, Rafał Miłecki wrote:
> > It's more reliable than guessing based on ECC strength. It allows using
> > NAND on devices with BCH-1 (e.g. D-Link DIR-885L).
> >
> > Signed-off-by: Rafał Miłecki <[email protected]>
> > ---
> > 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 c3331ff..dcb22dc 100644
> > --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> > @@ -1927,7 +1927,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)
>
> This doesn't handle most of the problems I noted on the early version of
> this series. (But thank you for following through on the algorithm
> selection refactoring!)
>
> Particularly, this change will
> (a) break any existing DTs which used to have 'nand-ecc-size = <1>', and
> would assume this gets Hamming ECC; and
> (b) allows DTs to specify Hamming ECC for unsupported modes, like 1024B
> sectors, or ecc_level != 1. None of these are supported in HW.

Indeed.

>
> > cfg->ecc_level = 15;
> > else
> > cfg->ecc_level = chip->ecc.strength;
>
> Something like the following probably works better (not tested):
>
> ---8<---
>
> From: Brian Norris <[email protected]>
> Date: Mon, 25 Apr 2016 20:48:02 -0700
> Subject: [PATCH] mtd: brcmnand: respect ECC algorithm set by the NAND
> subsystem
>
> This is more obvious than guessing based on ECC strength. It allows
> using NAND on devices with BCH-1 (e.g. D-Link DIR-885L).
>
> This maintains DT backward compatibility by defaulting to Hamming if a
> 1-bit ECC algorithm is specified without a corresponding algorithm
> selection. i.e., to use BCH-1, you must specify:
>
> nand-ecc-strength = <1>;
> nand-ecc-step-size = <512>;
> nand-ecc-algo = "bch";
>
> Also adds a check to ensure we haven't allowed someone to get by with SW
> ECC. If we want to support SW ECC, we need to refactor some other pieces
> of this driver.

I'm waiting a bit before applying this patch (I'd like to have a
Tested-by first).

>
> Signed-off-by: Brian Norris <[email protected]>
> ---
> drivers/mtd/nand/brcmnand/brcmnand.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index c3331ffcaffd..b76ad7c0144f 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1925,9 +1925,31 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> cfg->col_adr_bytes = 2;
> cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
>
> + if (chip->ecc.mode != NAND_ECC_HW) {
> + dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> + chip->ecc.mode);
> + return -EINVAL;
> + }
> +
> + if (chip->ecc.algo == NAND_ECC_UNKNOWN) {
> + if (chip->ecc.strength == 1 && chip->ecc.size == 512)
> + /* Default to Hamming for 1-bit ECC, if unspecified */
> + chip->ecc.algo = NAND_ECC_HAMMING;
> + else
> + /* Otherwise, BCH */
> + chip->ecc.algo = NAND_ECC_BCH;
> + }
> +
> + if (chip->ecc.algo == NAND_ECC_HAMMING && (chip->ecc.strength != 1 ||
> + chip->ecc.size != 512)) {
> + dev_err(ctrl->dev, "invalid Hamming params: %d bits per %d bytes\n",
> + chip->ecc.strength, chip->ecc.size);
> + return -EINVAL;
> + }
> +
> 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;



--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2016-04-26 18:38:12

by Rafał Miłecki

[permalink] [raw]
Subject: Re: [PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem

On 26 April 2016 at 07:53, Brian Norris <[email protected]> wrote:
> From: Brian Norris <[email protected]>
> Date: Mon, 25 Apr 2016 20:48:02 -0700
> Subject: [PATCH] mtd: brcmnand: respect ECC algorithm set by the NAND
> subsystem
>
> This is more obvious than guessing based on ECC strength. It allows
> using NAND on devices with BCH-1 (e.g. D-Link DIR-885L).
>
> This maintains DT backward compatibility by defaulting to Hamming if a
> 1-bit ECC algorithm is specified without a corresponding algorithm
> selection. i.e., to use BCH-1, you must specify:
>
> nand-ecc-strength = <1>;
> nand-ecc-step-size = <512>;
> nand-ecc-algo = "bch";
>
> Also adds a check to ensure we haven't allowed someone to get by with SW
> ECC. If we want to support SW ECC, we need to refactor some other pieces
> of this driver.
>
> Signed-off-by: Brian Norris <[email protected]>

Tested-by: Rafał Miłecki <[email protected]>

I just needed to apply following patch first:
[PATCH] mtd: nand: fix NULL pointer dereference in of_get_nand_ecc_algo

2016-04-27 07:55:29

by Boris Brezillon

[permalink] [raw]
Subject: Re: [PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem

On Mon, 25 Apr 2016 22:53:55 -0700
Brian Norris <[email protected]> wrote:

> From: Brian Norris <[email protected]>
> Date: Mon, 25 Apr 2016 20:48:02 -0700
> Subject: [PATCH] mtd: brcmnand: respect ECC algorithm set by the NAND
> subsystem
>
> This is more obvious than guessing based on ECC strength. It allows
> using NAND on devices with BCH-1 (e.g. D-Link DIR-885L).
>
> This maintains DT backward compatibility by defaulting to Hamming if a
> 1-bit ECC algorithm is specified without a corresponding algorithm
> selection. i.e., to use BCH-1, you must specify:
>
> nand-ecc-strength = <1>;
> nand-ecc-step-size = <512>;
> nand-ecc-algo = "bch";
>
> Also adds a check to ensure we haven't allowed someone to get by with SW
> ECC. If we want to support SW ECC, we need to refactor some other pieces
> of this driver.
>
> Signed-off-by: Brian Norris <[email protected]>

Applied, thanks.

Boris

> ---
> drivers/mtd/nand/brcmnand/brcmnand.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index c3331ffcaffd..b76ad7c0144f 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1925,9 +1925,31 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> cfg->col_adr_bytes = 2;
> cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
>
> + if (chip->ecc.mode != NAND_ECC_HW) {
> + dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> + chip->ecc.mode);
> + return -EINVAL;
> + }
> +
> + if (chip->ecc.algo == NAND_ECC_UNKNOWN) {
> + if (chip->ecc.strength == 1 && chip->ecc.size == 512)
> + /* Default to Hamming for 1-bit ECC, if unspecified */
> + chip->ecc.algo = NAND_ECC_HAMMING;
> + else
> + /* Otherwise, BCH */
> + chip->ecc.algo = NAND_ECC_BCH;
> + }
> +
> + if (chip->ecc.algo == NAND_ECC_HAMMING && (chip->ecc.strength != 1 ||
> + chip->ecc.size != 512)) {
> + dev_err(ctrl->dev, "invalid Hamming params: %d bits per %d bytes\n",
> + chip->ecc.strength, chip->ecc.size);
> + return -EINVAL;
> + }
> +
> 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;



--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com