[PATCH 0/3] mtd: nand: add enum nand_ecc_algo

[PATCH 0/3] mtd: nand: add enum nand_ecc_algo

[Rafał Miłecki]

Some time ago I started working on a new "nand-ecc-algo" property to allow
specifying ECC algorithm for hardware ECC mode as well.
Boris pointed out it's becoming a bit messy that way as we already have value
NAND_ECC_SOFT_BCH.
I suggested deprecating "soft_bch" value from nand-ecc-mode property and got
Boris agreed and no objections from others. So there is how I want to implement
this.
If you agree on this way & apply my patches, I'll start modifying NAND drivers
(similarly to the nandsim) and then will try to drop NAND_ECC_SOFT_BCH except
for handling backward compatibility.
Then finally we should be able to add "nand-ecc-algo" property support properly.

Rafał Miłecki (3):
  mtd: nand: add new enum for storing ECC algorithm
  mtd: nand: set ECC algorithm in nand_dt_init
  mtd: nand: nandsim: set ECC algorithm explicitly

 drivers/mtd/nand/nand_base.c | 15 ++++++++++++++-
 drivers/mtd/nand/nandsim.c   |  2 ++
 include/linux/mtd/nand.h     |  7 +++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

-- 
1.8.4.5

[PATCH 1/3] mtd: nand: add new enum for storing ECC algorithm

[Rafał Miłecki]

Our nand_ecc_modes_t is already a bit abused by value NAND_ECC_SOFT_BCH.
This enum should store ECC mode only and putting algorithm details there
is a bit idea. It would result in too many values impossible to support
in a sane way.

To solve this problem let's add a new enum. We'll have to modify all
drivers to set it properly but once it's done it'll be possible to drop
NAND_ECC_SOFT_BCH. That will result in a cleaner design and more
possibilities like setting ECC algorithm for hardware ECC mode.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 include/linux/mtd/nand.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7604f4b..b818eb3 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -119,6 +119,12 @@ typedef enum {
 	NAND_ECC_SOFT_BCH,
 } nand_ecc_modes_t;
 
+enum nand_ecc_algo {
+	NAND_ECC_UNKNOWN,
+	NAND_ECC_HAMMING,
+	NAND_ECC_BCH,
+};
+
 /*
  * Constants for Hardware ECC
  */
@@ -508,6 +514,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

[PATCH 2/3] mtd: nand: set ECC algorithm in nand_dt_init

[Rafał Miłecki]

Right now we set it based on "nand-ecc-mode" property value. At some
point we will most likely want a new property, but we'll need to keep
backward compatibility, so such code will be needed anyway.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/mtd/nand/nand_base.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index f2c8ff3..75c5564 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,16 @@ static int nand_dt_init(struct nand_chip *chip)
 		chip->bbt_options |= NAND_BBT_USE_FLASH;
 
 	ecc_mode = of_get_nand_ecc_mode(dn);
+	switch (ecc_mode) {
+	case NAND_ECC_SOFT:
+		ecc_algo = NAND_ECC_HAMMING;
+		break;
+	case NAND_ECC_SOFT_BCH:
+		ecc_algo = NAND_ECC_BCH;
+		break;
+	default:
+		ecc_algo = -1;
+	}
 	ecc_strength = of_get_nand_ecc_strength(dn);
 	ecc_step = of_get_nand_ecc_step_size(dn);
 
@@ -4003,6 +4013,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;
 
-- 
1.8.4.5

[PATCH 3/3] mtd: nand: nandsim: set ECC algorithm explicitly

[Rafał Miłecki]

This follows recent work on switching to enum nand_ecc_algo and
deprecating NAND_ECC_SOFT_BCH.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/mtd/nand/nandsim.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 1fd5195..6ff1d8d 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -2261,6 +2261,7 @@ static int __init ns_init_module(void)
 	chip->read_buf   = ns_nand_read_buf;
 	chip->read_word  = ns_nand_read_word;
 	chip->ecc.mode   = NAND_ECC_SOFT;
+	chip->ecc.algo   = NAND_ECC_HAMMING;
 	/* The NAND_SKIP_BBTSCAN option is necessary for 'overridesize' */
 	/* and 'badblocks' parameters to work */
 	chip->options   |= NAND_SKIP_BBTSCAN;
@@ -2339,6 +2340,7 @@ static int __init ns_init_module(void)
 			goto error;
 		}
 		chip->ecc.mode = NAND_ECC_SOFT_BCH;
+		chip->ecc.algo = NAND_ECC_BCH;
 		chip->ecc.size = 512;
 		chip->ecc.strength = bch;
 		chip->ecc.bytes = eccbytes;
-- 
1.8.4.5

Re: [PATCH 0/3] mtd: nand: add enum nand_ecc_algo

[Boris Brezillon]

Hi Rafal,

On Tue, 22 Mar 2016 23:54:11 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

> Some time ago I started working on a new "nand-ecc-algo" property to allow
> specifying ECC algorithm for hardware ECC mode as well.
> Boris pointed out it's becoming a bit messy that way as we already have value
> NAND_ECC_SOFT_BCH.
> I suggested deprecating "soft_bch" value from nand-ecc-mode property and got
> Boris agreed and no objections from others. So there is how I want to implement
> this.
> If you agree on this way & apply my patches, I'll start modifying NAND drivers
> (similarly to the nandsim) and then will try to drop NAND_ECC_SOFT_BCH except
> for handling backward compatibility.
> Then finally we should be able to add "nand-ecc-algo" property support properly.

I agree with patch 1 and 3, but I think we should create an
of_get_nand_ecc_algo() helper right now instead of relying on the
ecc->mode value to set ecc->algo (I'm referring to patch 2).

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) {
		if (!of_property_read_string(np, "nand-ecc-mode", &pm)){
			if (!strcasecmp(pm, "soft"))
				err = NAND_ECC_HAMMING;
			else if (!strcasecmp(pm, "soft_bch"))
				err = NAND_ECC_BCH;
		}

		return err;
	}

	for (i = 0; i < ARRAY_SIZE(nand_ecc_algos); i++)
		if (!strcasecmp(pm, nand_ecc_algos[i]))
			return i;

	return -EINVAL;
}
EXPORT_SYMBOL_GPL(of_get_nand_ecc_algo);

This way you'll be able to completely drop NAND_ECC_SOFT_BCH value when
all drivers have been patched.
BTW, even if we deprecate the DT property, we still have to support the
old format, so this extra "nand-ecc-mode" parsing will remain anyway.

And has already said ealier, I think we should also move the ecc layout
info somewhere else, and only leave ECC_NONE, ECC_HW and ECC_SOFT in
nand_ecc_modes_t enum. I'm not asking you to do that though, just
clarifying the separation I'd like to see ;).

Best Regards,

Boris

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

Re: [PATCH 0/3] mtd: nand: add enum nand_ecc_algo

[Rafał Miłecki]

On 23 March 2016 at 10:00, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 22 Mar 2016 23:54:11 +0100
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>> Some time ago I started working on a new "nand-ecc-algo" property to allow
>> specifying ECC algorithm for hardware ECC mode as well.
>> Boris pointed out it's becoming a bit messy that way as we already have value
>> NAND_ECC_SOFT_BCH.
>> I suggested deprecating "soft_bch" value from nand-ecc-mode property and got
>> Boris agreed and no objections from others. So there is how I want to implement
>> this.
>> If you agree on this way & apply my patches, I'll start modifying NAND drivers
>> (similarly to the nandsim) and then will try to drop NAND_ECC_SOFT_BCH except
>> for handling backward compatibility.
>> Then finally we should be able to add "nand-ecc-algo" property support properly.
>
> I agree with patch 1 and 3, but I think we should create an
> of_get_nand_ecc_algo() helper right now instead of relying on the
> ecc->mode value to set ecc->algo (I'm referring to patch 2).

Thanks for the review!

I wanted to avoid adding support for "nand-ecc-algo" at this point.
This would result with setting ecc.algo which isn't used yet, so this
could be confusing.
However I agree with your point that adding this magic code to
nand_dt_init, just to replace it with of_get_nand_ecc_algo in the
future isn't too clever.

I'll send V2 soon, let's see if will agree on its implementation.


> This way you'll be able to completely drop NAND_ECC_SOFT_BCH value when
> all drivers have been patched.
> BTW, even if we deprecate the DT property, we still have to support the
> old format, so this extra "nand-ecc-mode" parsing will remain anyway.

This is what I mainly meant in "then will try to drop
NAND_ECC_SOFT_BCH except for handling backward compatibility" :)


> And has already said ealier, I think we should also move the ecc layout
> info somewhere else, and only leave ECC_NONE, ECC_HW and ECC_SOFT in
> nand_ecc_modes_t enum. I'm not asking you to do that though, just
> clarifying the separation I'd like to see ;).

Ack, hopefully I'll find time for this later.

-- 
Rafał