[PATCH v8 0/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup

[PATCH v8 0/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup

[Pekon Gupta]

*changes v7 -> v8*
Incorporated feedbacks from Brian Norris <computersforpeace@gmail.com>
 - renamed ecc_step_bytes -> ecc_syndrome_size


*changes v6 -> v7*
Incorporated feedbacks from Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
 - using dev_err() instead of pr_err()
 - moved un-related addition of info->ecc_steps, info->ecc_step_bytes,
   info->ecc_step_size from [PATCH v6 1/4] into subsequent patches
 - dropped pr_fmt() change


*changes v5 -> v6*
[PATCH 02/04] minor cleanup

*changes v4 -> v5*
This patch series is split version from earlier series [1].
This series refactors and cleans ELM driver which is used by Hardware
based BCHx ecc-schemes.
 - Undo: introduction of 'struct mtd_info' and 'struct nand_chip'. Instead
         keep ELM driver independent of mtd_info and nand_chip structs and
         pass only required ECC configurations as elm_config() arguments
         elm_config(..., int ecc_steps, int ecc_step_size, int ecc_step_bytes)
 - Undo: re-writing of elm_load_syndrome() ECC register configurations.

*changes v3 -> v4 [1]*
 - in-corporated feedbacks from Brian Norris <computersforpeace@gmail.com>
 - updated: use 'pr_fmt(fmt)' to suffix DRIVER_NAME
 - removed: local 'eccsteps' in ELM driver, instead using nand_chip->ecc.steps
 - undo: irrelavant white-space changes

[1] http://lists.infradead.org/pipermail/linux-mtd/2013-November/050242.html

Pekon Gupta (4):
  mtd: devices: elm: check for hardware engine's design constraints
  mtd: devices: elm: clean elm_load_syndrome
  mtd: devices: elm: configure parallel channels based on ecc_steps
  mtd: devices: elm: update DRIVER_NAME as "omap-elm"

 drivers/mtd/devices/elm.c         | 43 ++++++++++++++++++++++++++++-----------
 drivers/mtd/nand/omap2.c          |  9 +++++---
 include/linux/platform_data/elm.h | 10 ++-------
 3 files changed, 39 insertions(+), 23 deletions(-)

-- 
1.8.5.1.163.gd7aced9

[PATCH v8 1/4] mtd: devices: elm: check for hardware engine's design constraints

[Pekon Gupta]

ELM hardware engine is used by BCH ecc-schemes for detecting and locating ECC
errors. This patch adds the following checks for ELM hardware engine:

 - ELM internal buffers are of 1K,
   so it cannot process data with ecc-step-size > 1K.

 - ELM engine can execute upto maximum of 8 threads in parallel,
   so in *page-mode* (when complete page is processed in single iteration),
   ELM cannot support ecc-steps > 8.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/devices/elm.c         | 13 ++++++++++++-
 drivers/mtd/nand/omap2.c          |  9 ++++++---
 include/linux/platform_data/elm.h |  3 ++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index f160d2c..c51752a 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -103,7 +103,8 @@ static u32 elm_read_reg(struct elm_info *info, int offset)
  * @dev:	ELM device
  * @bch_type:	Type of BCH ecc
  */
-int elm_config(struct device *dev, enum bch_ecc bch_type)
+int elm_config(struct device *dev, enum bch_ecc bch_type,
+	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
 {
 	u32 reg_val;
 	struct elm_info *info = dev_get_drvdata(dev);
@@ -112,6 +113,16 @@ int elm_config(struct device *dev, enum bch_ecc bch_type)
 		dev_err(dev, "Unable to configure elm - device not probed?\n");
 		return -ENODEV;
 	}
+	/* ELM cannot detect ECC errors for chunks > 1KB */
+	if (ecc_step_size > ((ELM_ECC_SIZE + 1) / 2)) {
+		dev_err(dev, "unsupported config ecc-size=%d\n", ecc_step_size);
+		return -EINVAL;
+	}
+	/* ELM support 8 error syndrome process */
+	if (ecc_steps > ERROR_VECTOR_MAX) {
+		dev_err(dev, "unsupported config ecc-step=%d\n", ecc_steps);
+		return -EINVAL;
+	}
 
 	reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
 	elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val);
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index ab9c472..6f9b339 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1540,6 +1540,8 @@ static int is_elm_present(struct omap_nand_info *info,
 			struct device_node *elm_node, enum bch_ecc bch_type)
 {
 	struct platform_device *pdev;
+	struct nand_ecc_ctrl *ecc = &info->nand.ecc;
+	int err;
 	/* check whether elm-id is passed via DT */
 	if (!elm_node) {
 		pr_err("nand: error: ELM DT node not found\n");
@@ -1553,9 +1555,10 @@ static int is_elm_present(struct omap_nand_info *info,
 	}
 	/* ELM module available, now configure it */
 	info->elm_dev = &pdev->dev;
-	if (elm_config(info->elm_dev, bch_type))
-		return -ENODEV;
-	return 0;
+	err = elm_config(info->elm_dev, bch_type,
+		(info->mtd.writesize / ecc->size), ecc->size, ecc->bytes);
+
+	return err;
 }
 #endif /* CONFIG_MTD_NAND_ECC_BCH */
 
diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
index bf0a83b..6e37156 100644
--- a/include/linux/platform_data/elm.h
+++ b/include/linux/platform_data/elm.h
@@ -50,5 +50,6 @@ struct elm_errorvec {
 
 void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 		struct elm_errorvec *err_vec);
-int elm_config(struct device *dev, enum bch_ecc bch_type);
+int elm_config(struct device *dev, enum bch_ecc bch_type,
+	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
 #endif /* __ELM_H */
-- 
1.8.5.1.163.gd7aced9

[PATCH v8 2/4] mtd: devices: elm: clean elm_load_syndrome

[Pekon Gupta]

This patch refactors elm_load_syndrome() to make it scalable for newer
ECC schemes by removing scheme specific macros (like ECC_BYTES*xx),
and instead using ECC control information passed during elm_config.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/devices/elm.c         | 18 +++++++++++-------
 include/linux/platform_data/elm.h |  7 -------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index c51752a..4fbfaf6 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -84,6 +84,7 @@ struct elm_info {
 	struct list_head list;
 	enum bch_ecc bch_type;
 	struct elm_registers elm_regs;
+	int ecc_syndrome_size;
 };
 
 static LIST_HEAD(elm_devices);
@@ -126,7 +127,8 @@ int elm_config(struct device *dev, enum bch_ecc bch_type,
 
 	reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
 	elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val);
-	info->bch_type = bch_type;
+	info->bch_type		= bch_type;
+	info->ecc_syndrome_size	= ecc_syndrome_size;
 
 	return 0;
 }
@@ -175,10 +177,8 @@ static void elm_load_syndrome(struct elm_info *info,
 			elm_configure_page_mode(info, i, true);
 			offset = ELM_SYNDROME_FRAGMENT_0 +
 				SYNDROME_FRAGMENT_REG_SIZE * i;
-
-			/* BCH8 */
-			if (info->bch_type) {
-
+			switch (info->bch_type) {
+			case BCH8_ECC:
 				/* syndrome fragment 0 = ecc[9-12B] */
 				val = cpu_to_be32(*(u32 *) &ecc[9]);
 				elm_write_reg(info, offset, val);
@@ -197,7 +197,8 @@ static void elm_load_syndrome(struct elm_info *info,
 				offset += 4;
 				val = ecc[0];
 				elm_write_reg(info, offset, val);
-			} else {
+				break;
+			case BCH4_ECC:
 				/* syndrome fragment 0 = ecc[20-52b] bits */
 				val = (cpu_to_be32(*(u32 *) &ecc[3]) >> 4) |
 					((ecc[2] & 0xf) << 28);
@@ -207,11 +208,14 @@ static void elm_load_syndrome(struct elm_info *info,
 				offset += 4;
 				val = cpu_to_be32(*(u32 *) &ecc[0]) >> 12;
 				elm_write_reg(info, offset, val);
+				break;
+			default:
+				pr_err("invalid config bch_type\n");
 			}
 		}
 
 		/* Update ecc pointer with ecc byte size */
-		ecc += info->bch_type ? BCH8_SIZE : BCH4_SIZE;
+		ecc += info->ecc_syndrome_size;
 	}
 }
 
diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
index 6e37156..4edb406 100644
--- a/include/linux/platform_data/elm.h
+++ b/include/linux/platform_data/elm.h
@@ -26,13 +26,6 @@ enum bch_ecc {
 /* ELM support 8 error syndrome process */
 #define ERROR_VECTOR_MAX		8
 
-#define BCH8_ECC_OOB_BYTES		13
-#define BCH4_ECC_OOB_BYTES		7
-/* RBL requires 14 byte even though BCH8 uses only 13 byte */
-#define BCH8_SIZE			(BCH8_ECC_OOB_BYTES + 1)
-/* Uses 1 extra byte to handle erased pages */
-#define BCH4_SIZE			(BCH4_ECC_OOB_BYTES + 1)
-
 /**
  * struct elm_errorvec - error vector for elm
  * @error_reported:		set true for vectors error is reported
-- 
1.8.5.1.163.gd7aced9

[PATCH v8 3/4] mtd: devices: elm: configure parallel channels based on ecc_steps

[Pekon Gupta]

ELM hardware can process up to maximum of 8 hannels in parallel for
ECC error detection. Currently the number of channels getting configured for
processing is static determined by macro ERROR_VECTOR_MAX. However, the actual
number of channels that need to be processed is the ECC step number.
This patch just avoids configuring extra unused channels.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/devices/elm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index 4fbfaf6..26df41f 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -84,6 +84,7 @@ struct elm_info {
 	struct list_head list;
 	enum bch_ecc bch_type;
 	struct elm_registers elm_regs;
+	int ecc_steps;
 	int ecc_syndrome_size;
 };
 
@@ -128,6 +129,7 @@ int elm_config(struct device *dev, enum bch_ecc bch_type,
 	reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
 	elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val);
 	info->bch_type		= bch_type;
+	info->ecc_steps		= ecc_steps;
 	info->ecc_syndrome_size	= ecc_syndrome_size;
 
 	return 0;
@@ -170,7 +172,7 @@ static void elm_load_syndrome(struct elm_info *info,
 	int i, offset;
 	u32 val;
 
-	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
+	for (i = 0; i < info->ecc_steps; i++) {
 
 		/* Check error reported */
 		if (err_vec[i].error_reported) {
@@ -238,7 +240,7 @@ static void elm_start_processing(struct elm_info *info,
 	 * Set syndrome vector valid, so that ELM module
 	 * will process it for vectors error is reported
 	 */
-	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
+	for (i = 0; i < info->ecc_steps; i++) {
 		if (err_vec[i].error_reported) {
 			offset = ELM_SYNDROME_FRAGMENT_6 +
 				SYNDROME_FRAGMENT_REG_SIZE * i;
@@ -267,7 +269,7 @@ static void elm_error_correction(struct elm_info *info,
 	int offset;
 	u32 reg_val;
 
-	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
+	for (i = 0; i < info->ecc_steps; i++) {
 
 		/* Check error reported */
 		if (err_vec[i].error_reported) {
-- 
1.8.5.1.163.gd7aced9

[PATCH v8 4/4] mtd: devices: elm: update DRIVER_NAME as "omap-elm"

[Pekon Gupta]

use "omap-elm" as DRIVER_NAME

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/devices/elm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index 26df41f..1fd4a0f 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -15,6 +15,8 @@
  *
  */
 
+#define DRIVER_NAME	"omap-elm"
+
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -520,7 +522,7 @@ MODULE_DEVICE_TABLE(of, elm_of_match);
 
 static struct platform_driver elm_driver = {
 	.driver	= {
-		.name	= "elm",
+		.name	= DRIVER_NAME,
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(elm_of_match),
 		.pm	= &elm_pm_ops,
-- 
1.8.5.1.163.gd7aced9

Re: [PATCH v8 0/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup

[Ezequiel Garcia]

On Mar 20, Pekon Gupta wrote:
> *changes v7 -> v8*
> Incorporated feedbacks from Brian Norris <computersforpeace@gmail.com>
>  - renamed ecc_step_bytes -> ecc_syndrome_size
> 
> 
> *changes v6 -> v7*
> Incorporated feedbacks from Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>  - using dev_err() instead of pr_err()
>  - moved un-related addition of info->ecc_steps, info->ecc_step_bytes,
>    info->ecc_step_size from [PATCH v6 1/4] into subsequent patches
>  - dropped pr_fmt() change
> 
> 
> *changes v5 -> v6*
> [PATCH 02/04] minor cleanup
> 
> *changes v4 -> v5*
> This patch series is split version from earlier series [1].
> This series refactors and cleans ELM driver which is used by Hardware
> based BCHx ecc-schemes.
>  - Undo: introduction of 'struct mtd_info' and 'struct nand_chip'. Instead
>          keep ELM driver independent of mtd_info and nand_chip structs and
>          pass only required ECC configurations as elm_config() arguments
>          elm_config(..., int ecc_steps, int ecc_step_size, int ecc_step_bytes)
>  - Undo: re-writing of elm_load_syndrome() ECC register configurations.
> 
> *changes v3 -> v4 [1]*
>  - in-corporated feedbacks from Brian Norris <computersforpeace@gmail.com>
>  - updated: use 'pr_fmt(fmt)' to suffix DRIVER_NAME
>  - removed: local 'eccsteps' in ELM driver, instead using nand_chip->ecc.steps
>  - undo: irrelavant white-space changes
> 
> [1] http://lists.infradead.org/pipermail/linux-mtd/2013-November/050242.html
> 
> Pekon Gupta (4):
>   mtd: devices: elm: check for hardware engine's design constraints
>   mtd: devices: elm: clean elm_load_syndrome
>   mtd: devices: elm: configure parallel channels based on ecc_steps
>   mtd: devices: elm: update DRIVER_NAME as "omap-elm"
> 

For the whole series:

Reviewed-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v8 0/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup

[Brian Norris]

On Thu, Mar 20, 2014 at 10:48:28AM -0300, Ezequiel Garcia wrote:
> On Mar 20, Pekon Gupta wrote:
> > *changes v7 -> v8*
> > Incorporated feedbacks from Brian Norris <computersforpeace@gmail.com>
> >  - renamed ecc_step_bytes -> ecc_syndrome_size
> > 
[...]
> > Pekon Gupta (4):
> >   mtd: devices: elm: check for hardware engine's design constraints
> >   mtd: devices: elm: clean elm_load_syndrome
> >   mtd: devices: elm: configure parallel channels based on ecc_steps
> >   mtd: devices: elm: update DRIVER_NAME as "omap-elm"
> > 
> 
> For the whole series:
> 
> Reviewed-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Pushed to l2-mtd.git. Thanks!

Brian

[PATCH v8 1/4] mtd: devices: elm: check for hardware engine's design constraints

[Pekon Gupta]

ELM hardware engine is used by BCH ecc-schemes for detecting and locating ECC
errors. This patch adds the following checks for ELM hardware engine:

 - ELM internal buffers are of 1K,
   so it cannot process data with ecc-step-size > 1K.

 - ELM engine can execute upto maximum of 8 threads in parallel,
   so in *page-mode* (when complete page is processed in single iteration),
   ELM cannot support ecc-steps > 8.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/devices/elm.c         | 13 ++++++++++++-
 drivers/mtd/nand/omap2.c          |  9 ++++++---
 include/linux/platform_data/elm.h |  3 ++-
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index f160d2c..c51752a 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -103,7 +103,8 @@ static u32 elm_read_reg(struct elm_info *info, int offset)
  * @dev:	ELM device
  * @bch_type:	Type of BCH ecc
  */
-int elm_config(struct device *dev, enum bch_ecc bch_type)
+int elm_config(struct device *dev, enum bch_ecc bch_type,
+	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
 {
 	u32 reg_val;
 	struct elm_info *info = dev_get_drvdata(dev);
@@ -112,6 +113,16 @@ int elm_config(struct device *dev, enum bch_ecc bch_type)
 		dev_err(dev, "Unable to configure elm - device not probed?\n");
 		return -ENODEV;
 	}
+	/* ELM cannot detect ECC errors for chunks > 1KB */
+	if (ecc_step_size > ((ELM_ECC_SIZE + 1) / 2)) {
+		dev_err(dev, "unsupported config ecc-size=%d\n", ecc_step_size);
+		return -EINVAL;
+	}
+	/* ELM support 8 error syndrome process */
+	if (ecc_steps > ERROR_VECTOR_MAX) {
+		dev_err(dev, "unsupported config ecc-step=%d\n", ecc_steps);
+		return -EINVAL;
+	}
 
 	reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
 	elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val);
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index ab9c472..6f9b339 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1540,6 +1540,8 @@ static int is_elm_present(struct omap_nand_info *info,
 			struct device_node *elm_node, enum bch_ecc bch_type)
 {
 	struct platform_device *pdev;
+	struct nand_ecc_ctrl *ecc = &info->nand.ecc;
+	int err;
 	/* check whether elm-id is passed via DT */
 	if (!elm_node) {
 		pr_err("nand: error: ELM DT node not found\n");
@@ -1553,9 +1555,10 @@ static int is_elm_present(struct omap_nand_info *info,
 	}
 	/* ELM module available, now configure it */
 	info->elm_dev = &pdev->dev;
-	if (elm_config(info->elm_dev, bch_type))
-		return -ENODEV;
-	return 0;
+	err = elm_config(info->elm_dev, bch_type,
+		(info->mtd.writesize / ecc->size), ecc->size, ecc->bytes);
+
+	return err;
 }
 #endif /* CONFIG_MTD_NAND_ECC_BCH */
 
diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
index bf0a83b..6e37156 100644
--- a/include/linux/platform_data/elm.h
+++ b/include/linux/platform_data/elm.h
@@ -50,5 +50,6 @@ struct elm_errorvec {
 
 void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 		struct elm_errorvec *err_vec);
-int elm_config(struct device *dev, enum bch_ecc bch_type);
+int elm_config(struct device *dev, enum bch_ecc bch_type,
+	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
 #endif /* __ELM_H */
-- 
1.8.5.1.163.gd7aced9