* [PATCH 1/3] mtd: nand: add new enum for storing ECC algorithm
2016-03-22 22:54 [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Rafał Miłecki
@ 2016-03-22 22:54 ` Rafał Miłecki
2016-03-22 22:54 ` [PATCH 2/3] mtd: nand: set ECC algorithm in nand_dt_init Rafał Miłecki
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2016-03-22 22:54 UTC (permalink / raw)
To: Brian Norris, linux-mtd; +Cc: Boris Brezillon, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] mtd: nand: set ECC algorithm in nand_dt_init
2016-03-22 22:54 [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Rafał Miłecki
2016-03-22 22:54 ` [PATCH 1/3] mtd: nand: add new enum for storing ECC algorithm Rafał Miłecki
@ 2016-03-22 22:54 ` Rafał Miłecki
2016-03-22 22:54 ` [PATCH 3/3] mtd: nand: nandsim: set ECC algorithm explicitly Rafał Miłecki
2016-03-23 9:00 ` [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Boris Brezillon
3 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2016-03-22 22:54 UTC (permalink / raw)
To: Brian Norris, linux-mtd; +Cc: Boris Brezillon, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] mtd: nand: nandsim: set ECC algorithm explicitly
2016-03-22 22:54 [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Rafał Miłecki
2016-03-22 22:54 ` [PATCH 1/3] mtd: nand: add new enum for storing ECC algorithm Rafał Miłecki
2016-03-22 22:54 ` [PATCH 2/3] mtd: nand: set ECC algorithm in nand_dt_init Rafał Miłecki
@ 2016-03-22 22:54 ` Rafał Miłecki
2016-03-23 9:00 ` [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Boris Brezillon
3 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2016-03-22 22:54 UTC (permalink / raw)
To: Brian Norris, linux-mtd; +Cc: Boris Brezillon, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] mtd: nand: add enum nand_ecc_algo
2016-03-22 22:54 [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Rafał Miłecki
` (2 preceding siblings ...)
2016-03-22 22:54 ` [PATCH 3/3] mtd: nand: nandsim: set ECC algorithm explicitly Rafał Miłecki
@ 2016-03-23 9:00 ` Boris Brezillon
2016-03-23 9:10 ` Rafał Miłecki
3 siblings, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2016-03-23 9:00 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: Brian Norris, linux-mtd
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] mtd: nand: add enum nand_ecc_algo
2016-03-23 9:00 ` [PATCH 0/3] mtd: nand: add enum nand_ecc_algo Boris Brezillon
@ 2016-03-23 9:10 ` Rafał Miłecki
0 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2016-03-23 9:10 UTC (permalink / raw)
To: Boris Brezillon; +Cc: Brian Norris, linux-mtd@lists.infradead.org
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ł
^ permalink raw reply [flat|nested] 6+ messages in thread