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

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

[Pekon Gupta]

*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 constrains
  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         | 44 +++++++++++++++++++++++++++++----------
 drivers/mtd/nand/omap2.c          |  9 +++++---
 include/linux/platform_data/elm.h |  3 ++-
 3 files changed, 41 insertions(+), 15 deletions(-)

-- 
1.8.5.1.163.gd7aced9

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

[Pekon Gupta]

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

 - 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         | 19 ++++++++++++++++++-
 drivers/mtd/nand/omap2.c          |  9 ++++++---
 include/linux/platform_data/elm.h |  3 ++-
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index f160d2c..7fda50f 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -84,6 +84,9 @@ struct elm_info {
 	struct list_head list;
 	enum bch_ecc bch_type;
 	struct elm_registers elm_regs;
+	int ecc_steps;
+	int ecc_step_size;
+	int ecc_step_bytes;
 };
 
 static LIST_HEAD(elm_devices);
@@ -103,7 +106,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_step_bytes)
 {
 	u32 reg_val;
 	struct elm_info *info = dev_get_drvdata(dev);
@@ -112,10 +116,23 @@ 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/2 + 1)) {
+		pr_err("unsupported config ecc-size=%d", ecc_step_size);
+		return -EINVAL;
+	}
+	/* ELM support 8 error syndrome process */
+	if (ecc_steps > ERROR_VECTOR_MAX) {
+		pr_err("unsupported config ecc-step=%d", 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);
 	info->bch_type = bch_type;
+	info->ecc_steps		= ecc_steps;
+	info->ecc_step_size	= ecc_step_size;
+	info->ecc_step_bytes	= ecc_step_bytes;
 
 	return 0;
 }
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5ce2097..369aee7 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1546,6 +1546,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 = 0;
 	/* check whether elm-id is passed via DT */
 	if (!elm_node) {
 		pr_err("nand: error: ELM DT node not found\n");
@@ -1559,9 +1561,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..b824ff3 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_step_bytes);
 #endif /* __ELM_H */
-- 
1.8.5.1.163.gd7aced9

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

[Pekon Gupta]

 - replace ECC_BYTES* macros with ecc->bytes
 - Add scalability for future ecc-schemes

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

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index 7fda50f..f59c100 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -181,10 +181,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);
@@ -203,7 +201,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);
@@ -213,11 +212,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_step_bytes;
 	}
 }
 
-- 
1.8.5.1.163.gd7aced9

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

[Pekon Gupta]

Though ELM hardware can process upto maximum of 8 channels in parallel for
ECC error detection. But actual number of channels need to be processed in
parallel depends on ecc_steps (for page mode configuration).

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

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index f59c100..43fd81d 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -174,7 +174,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) {
@@ -242,7 +242,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;
@@ -271,7 +271,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 v5 4/4] mtd: devices: elm: update DRIVER_NAME as "omap-elm"

[Pekon Gupta]

use "omap-elm" as DRIVER_NAME
prefix DRIVER_NAME in debug and error messages

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

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index 43fd81d..16e62ae 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -15,6 +15,9 @@
  *
  */
 
+#define	DRIVER_NAME	"omap-elm"
+#define pr_fmt(fmt)	DRIVER_NAME ": " fmt
+
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -522,7 +525,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 v5 1/4] mtd: devices: elm: check for hardware engine's design constrains

[Brian Norris]

On Mon, Mar 03, 2014 at 04:49:20PM +0530, Pekon Gupta wrote:
> ELM hardware engine is used by BCH ecc-schemes for detecting and locating
> ECC errors. This patch adds following checks as per ELM design constrains:

s/constrains/constraints/

> 
>  - 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         | 19 ++++++++++++++++++-
>  drivers/mtd/nand/omap2.c          |  9 ++++++---
>  include/linux/platform_data/elm.h |  3 ++-
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
> index f160d2c..7fda50f 100644
> --- a/drivers/mtd/devices/elm.c
> +++ b/drivers/mtd/devices/elm.c
> @@ -84,6 +84,9 @@ struct elm_info {
>  	struct list_head list;
>  	enum bch_ecc bch_type;
>  	struct elm_registers elm_regs;
> +	int ecc_steps;
> +	int ecc_step_size;
> +	int ecc_step_bytes;

I'm not sure 'ecc_step_bytes' is the right name. It sounds too similar
to 'ecc_step_size', when in fact it means "ECC syndrome bytes per step",
right? Maybe just 'ecc_syndrome_size'?

>  };
>  
>  static LIST_HEAD(elm_devices);
> @@ -103,7 +106,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_step_bytes)
>  {
>  	u32 reg_val;
>  	struct elm_info *info = dev_get_drvdata(dev);
> @@ -112,10 +116,23 @@ 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/2 + 1)) {

Please add a space around the divide operator:

	if (ecc_step_size > (ELM_ECC_SIZE / 2 + 1)) {

> +		pr_err("unsupported config ecc-size=%d", ecc_step_size);

You're missing "\n" in your format string.

> +		return -EINVAL;
> +	}
> +	/* ELM support 8 error syndrome process */
> +	if (ecc_steps > ERROR_VECTOR_MAX) {
> +		pr_err("unsupported config ecc-step=%d", ecc_steps);

Also missing "\n" here.

> +		return -EINVAL;
> +	}
>  
>  	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_step_size	= ecc_step_size;
> +	info->ecc_step_bytes	= ecc_step_bytes;
>  
>  	return 0;
>  }
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5ce2097..369aee7 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1546,6 +1546,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 = 0;
>  	/* check whether elm-id is passed via DT */
>  	if (!elm_node) {
>  		pr_err("nand: error: ELM DT node not found\n");
> @@ -1559,9 +1561,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);

Isn't info->mtd.writesize / ecc->size the same as just ecc->steps?

> +
> +	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..b824ff3 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_step_bytes);
>  #endif /* __ELM_H */

Brian

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

[Brian Norris]

On Mon, Mar 03, 2014 at 04:49:21PM +0530, Pekon Gupta wrote:
>  - replace ECC_BYTES* macros with ecc->bytes

^^ this comment doesn't quite match. But the patch looks OK.

>  - Add scalability for future ecc-schemes
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/devices/elm.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
> index 7fda50f..f59c100 100644
> --- a/drivers/mtd/devices/elm.c
> +++ b/drivers/mtd/devices/elm.c
> @@ -181,10 +181,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);
> @@ -203,7 +201,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);
> @@ -213,11 +212,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_step_bytes;
>  	}
>  }
>  

Brian

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

[Brian Norris]

On Mon, Mar 03, 2014 at 04:49:23PM +0530, Pekon Gupta wrote:
> use "omap-elm" as DRIVER_NAME
> prefix DRIVER_NAME in debug and error messages
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/devices/elm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
> index 43fd81d..16e62ae 100644
> --- a/drivers/mtd/devices/elm.c
> +++ b/drivers/mtd/devices/elm.c
> @@ -15,6 +15,9 @@
>   *
>   */
>  
> +#define	DRIVER_NAME	"omap-elm"

Your spacing is a little off. You have a tab between 'define' and
'DRIVER_NAME'.

> +#define pr_fmt(fmt)	DRIVER_NAME ": " fmt
> +
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> @@ -522,7 +525,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,

Brian

RE: [PATCH v5 1/4] mtd: devices: elm: check for hardware engine's design constrains

[Gupta, Pekon]

Hi Brian,

>From: Brian Norris [mailto:computersforpeace@gmail.com]

Thanks a lot for picking up the other patch series.
These patches make OMAP NAND look cleaner, scalable and stable.

>> @@ -1559,9 +1561,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);
>
>Isn't info->mtd.writesize / ecc->size the same as just ecc->steps?
>
'ecc->steps' gets its value in nand_scan_tail(), but this particular
check on ELM driver, gets executed before nand_scan_tail() is called
in driver probe(), so have do the ecc->steps calculation myself.

I'll include all other comments in next version..

with regards, pekon