[PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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".
 - 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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".
 - 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

[PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Rafał Miłecki]

This will allow drivers handle ECC properly.

Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Rafał Miłecki]

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;
+
 	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

[PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm

[Rafał Miłecki]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm

[Rafał Miłecki]

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)
 			cfg->ecc_level = 15;
 		else
 			cfg->ecc_level = chip->ecc.strength;
-- 
1.8.4.5

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Kamal Dasu]

[-- Attachment #1: Type: text/plain, Size: 4333 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 6039 bytes --]

Re: [PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm

[Kamal Dasu]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm

[Kamal Dasu]

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
>

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Kamal Dasu]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Kamal Dasu]

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
>

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Kamal Dasu]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Kamal Dasu]

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
>

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rob Herring]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mtd/nand.txt |  3 +++

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rob Herring]

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(+)

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

Hi Rafal,

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.

Best Regards,

Boris

-- 
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

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

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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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.

-- 
Rafał
--
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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.

-- 
Rafał

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

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.

-- 
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

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.

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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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).

What about:
1) Adding new nand-ecc-algo as this patch does
2) Deprecating "soft_bch" value from nand-ecc-mode property

-- 
Rafał
--
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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

-- 
Rafał

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

On 26 February 2016 at 17:55, Boris Brezillon
<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Fri, 26 Feb 2016 17:30:47 +0100
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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?

-- 
Rafał
--
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Rafał Miłecki]

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?

-- 
Rafał

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

On Fri, 26 Feb 2016 22:24:21 +0100
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 26 February 2016 at 17:55, Boris Brezillon
> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Fri, 26 Feb 2016 17:30:47 +0100
> > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

-- 
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Boris Brezillon]

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.

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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Brian Norris]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Brian Norris <computersforpeace-dVM3bRS+LDk@public.gmane.org>
--
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

Re: [PATCH 1/3] of: mtd: add helper reading "nand-ecc-algo" from DT

[Brian Norris]

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>

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Brian Norris]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Brian Norris <computersforpeace-dVM3bRS+LDk@public.gmane.org>
--
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

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Brian Norris]

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>

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Brian Norris]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Brian Norris]

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
> 

Re: [PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm

[Brian Norris]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] mtd: brcmnand: fix check for Hamming algorithm

[Brian Norris]

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
> 

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Rafał Miłecki]

On 1 April 2016 at 18:07, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Rafał Miłecki]

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.

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Brian Norris]

On Fri, Apr 01, 2016 at 09:32:40PM +0200, Rafał Miłecki wrote:
> On 1 April 2016 at 18:07, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Brian Norris]

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

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Rafał Miłecki]

On 1 April 2016 at 22:07, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  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.

-- 
Rafał
--
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

Re: [PATCH 2/3] mtd: nand: read (from DT) and store ECC algorithm

[Rafał Miłecki]

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.

-- 
Rafał