Patchwork [v3,4/4] mtd: devices: elm: add checks ELM H/W constrains, driver code cleanup

login
register
mail settings
Submitter pekon gupta
Date Nov. 2, 2013, 9:46 a.m.
Message ID <1383385576-26095-5-git-send-email-pekon@ti.com>
Download mbox | patch
Permalink /patch/287959/
State New
Headers show

Comments

pekon gupta - Nov. 2, 2013, 9:46 a.m.
ELM H/W engine is used by BCHx_ECC schemes for detecting and locating bit-flips.
However, ELM H/W engine has some constrains like:
- ELM can decode errors in chunks of 512 data bytes only
- ELM can operate max upto 8 such buffers in parallel

This patch
- add checks for above constrains
- fixes ELM register configs based on number of info->eccsteps
- cleans-up elm_load_syndrome()

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/devices/elm.c         | 122 ++++++++++++++++++++++----------------
 drivers/mtd/nand/omap2.c          |   2 +-
 include/linux/platform_data/elm.h |   6 +-
 3 files changed, 77 insertions(+), 53 deletions(-)
Brian Norris - Nov. 13, 2013, 10:37 p.m.
On Sat, Nov 02, 2013 at 03:16:16PM +0530, Pekon Gupta wrote:
> ELM H/W engine is used by BCHx_ECC schemes for detecting and locating bit-flips.
> However, ELM H/W engine has some constrains like:
> - ELM can decode errors in chunks of 512 data bytes only
> - ELM can operate max upto 8 such buffers in parallel
> 
> This patch
> - add checks for above constrains
> - fixes ELM register configs based on number of info->eccsteps
> - cleans-up elm_load_syndrome()
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/devices/elm.c         | 122 ++++++++++++++++++++++----------------
>  drivers/mtd/nand/omap2.c          |   2 +-
>  include/linux/platform_data/elm.h |   6 +-
>  3 files changed, 77 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
> index d1dd6a3..2167384 100644
> --- a/drivers/mtd/devices/elm.c
> +++ b/drivers/mtd/devices/elm.c
> @@ -22,8 +22,11 @@
>  #include <linux/of.h>
>  #include <linux/sched.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
>  #include <linux/platform_data/elm.h>
>  
> +#define	DRIVER_NAME			"omap-elm"

The driver is actually named "elm", in the platform_driver struct.
Should you use the same name?

>  #define ELM_SYSCONFIG			0x010
>  #define ELM_IRQSTATUS			0x018
>  #define ELM_IRQENABLE			0x01c
> @@ -82,8 +85,10 @@ struct elm_info {
>  	void __iomem *elm_base;
>  	struct completion elm_completion;
>  	struct list_head list;
> +	struct mtd_info *mtd;
>  	enum bch_ecc bch_type;
>  	struct elm_registers elm_regs;
> +	int eccsteps;
>  };
>  
>  static LIST_HEAD(elm_devices);
> @@ -103,19 +108,42 @@ 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, struct mtd_info *mtd,
> +		enum bch_ecc bch_type)
>  {
>  	u32 reg_val;
> -	struct elm_info *info = dev_get_drvdata(dev);
> -
> +	struct elm_info	 *info;
> +	struct nand_chip *chip;
> +	if (!dev) {
> +		pr_err("%s: ELM device not found\n", DRIVER_NAME);

How about defining pr_fmt(), so you don't have to repeat the DRIVER_NAME
boiler-plate?

> +		return -ENODEV;
> +	}
> +	info = dev_get_drvdata(dev);
>  	if (!info) {
> -		dev_err(dev, "Unable to configure elm - device not probed?\n");
> +		pr_err("%s: ELM device data not found\n", DRIVER_NAME);
>  		return -ENODEV;
>  	}
> -
> +	if (!mtd) {
> +		pr_err("%s: MTD device not found\n", DRIVER_NAME);
> +		return -ENODEV;
> +	}
> +	chip = mtd->priv;
> +	/* ELM supports error correction in chunks of 512bytes of data only
> +	 * where each 512bytes of data has its own ECC syndrome */
> +	if (chip->ecc.size != 512) {
> +		pr_err("%s: invalid ecc_size configuration", DRIVER_NAME);
> +		return -EINVAL;
> +	}
> +	if (mtd->writesize > 4096) {
> +		pr_err("%s: page-size > 4096 is not supported", DRIVER_NAME);
> +		return -EINVAL;
> +	}
> +	/* ELM eccsteps required to decode complete NAND page */
> +	info->mtd	= mtd;
> +	info->bch_type	= bch_type;
> +	info->eccsteps = mtd->writesize / chip->ecc.size;

Isn't this the same as chip->ecc.steps? Do we need to recalculate it
here?

>  	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;
>  
>  	return 0;
>  }
> @@ -152,55 +180,51 @@ static void elm_configure_page_mode(struct elm_info *info, int index,
>   * Load syndrome fragment registers with calculated ecc in reverse order.
>   */
>  static void elm_load_syndrome(struct elm_info *info,
> -		struct elm_errorvec *err_vec, u8 *ecc)
> +		struct elm_errorvec *err_vec, u8 *ecc_calc)
>  {
> +	struct nand_chip *chip	= info->mtd->priv;
> +	unsigned int eccbytes	= chip->ecc.bytes;
> +	u8 *ecc = ecc_calc;
>  	int i, offset;
>  	u32 val;
>  
> -	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
> -
> +	for (i = 0; i < info->eccsteps; i++) {
>  		/* Check error reported */
>  		if (err_vec[i].error_reported) {
>  			elm_configure_page_mode(info, i, true);
> -			offset = ELM_SYNDROME_FRAGMENT_0 +
> -				SYNDROME_FRAGMENT_REG_SIZE * i;
> -
> -			/* BCH8 */
> -			if (info->bch_type) {
> -
> -				/* syndrome fragment 0 = ecc[9-12B] */
> -				val = cpu_to_be32(*(u32 *) &ecc[9]);
> -				elm_write_reg(info, offset, val);
> -
> -				/* syndrome fragment 1 = ecc[5-8B] */
> -				offset += 4;
> -				val = cpu_to_be32(*(u32 *) &ecc[5]);
> -				elm_write_reg(info, offset, val);
> -
> -				/* syndrome fragment 2 = ecc[1-4B] */
> -				offset += 4;
> -				val = cpu_to_be32(*(u32 *) &ecc[1]);
> -				elm_write_reg(info, offset, val);
> -
> -				/* syndrome fragment 3 = ecc[0B] */
> -				offset += 4;
> -				val = ecc[0];
> -				elm_write_reg(info, offset, val);
> -			} else {
> -				/* syndrome fragment 0 = ecc[20-52b] bits */
> -				val = (cpu_to_be32(*(u32 *) &ecc[3]) >> 4) |
> -					((ecc[2] & 0xf) << 28);
> -				elm_write_reg(info, offset, val);
> -
> -				/* syndrome fragment 1 = ecc[0-20b] bits */
> -				offset += 4;
> -				val = cpu_to_be32(*(u32 *) &ecc[0]) >> 12;
> -				elm_write_reg(info, offset, val);
> +			offset = SYNDROME_FRAGMENT_REG_SIZE * i;
> +			ecc = ecc_calc + (i * eccbytes);
> +			switch (info->bch_type) {
> +			case BCH4_ECC:
> +				val =	((*(ecc + 6) >>  4) & 0x0F) |
> +					*(ecc +  5) <<  4 | *(ecc +  4) << 12 |
> +					*(ecc +  3) << 20 | *(ecc +  2) << 28;
> +				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_0 +
> +						 offset), cpu_to_le32(val));
> +				val =	((*(ecc + 2) >>  4) & 0x0F) |
> +					*(ecc +  1) <<  4 | *(ecc +  0) << 12;
> +				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_1 +
> +						 offset), cpu_to_le32(val));
> +				break;
> +			case BCH8_ECC:
> +				val =	*(ecc + 12) << 0  | *(ecc + 11) <<  8 |
> +					*(ecc + 10) << 16 | *(ecc +  9) << 24;
> +				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_0 +
> +						 offset), cpu_to_le32(val));
> +				val =	*(ecc +  8) <<  0 | *(ecc +  7) <<  8 |
> +					*(ecc +  6) << 16 | *(ecc +  5) << 24;
> +				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_1 +
> +						 offset), cpu_to_le32(val));
> +				val =	*(ecc +  4) <<  0 | *(ecc +  3) <<  8 |
> +					*(ecc +  2) << 16 | *(ecc +  1) << 24;
> +				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_2 +
> +						 offset), cpu_to_le32(val));
> +				val =	*(ecc +  0) <<  0 & 0x000000FF;
> +				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_3 +
> +						 offset), cpu_to_le32(val));
> +				break;

That's some "interesting" shifting logic. But I think it looks OK...

>  			}
>  		}
> -
> -		/* Update ecc pointer with ecc byte size */
> -		ecc += info->bch_type ? BCH8_SIZE : BCH4_SIZE;
>  	}
>  }
>  
> @@ -223,7 +247,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->eccsteps; i++) {
>  		if (err_vec[i].error_reported) {
>  			offset = ELM_SYNDROME_FRAGMENT_6 +
>  				SYNDROME_FRAGMENT_REG_SIZE * i;
> @@ -252,7 +276,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->eccsteps; i++) {
>  
>  		/* Check error reported */
>  		if (err_vec[i].error_reported) {
> @@ -263,14 +287,12 @@ static void elm_error_correction(struct elm_info *info,
>  			if (reg_val & ECC_CORRECTABLE_MASK) {
>  				offset = ELM_ERROR_LOCATION_0 +
>  					ERROR_LOCATION_SIZE * i;
> -

Random whitespace change? Might not belong in this patch.

>  				/* Read count of correctable errors */
>  				err_vec[i].error_count = reg_val &
>  					ECC_NB_ERRORS_MASK;
>  
>  				/* Update the error locations in error vector */
>  				for (j = 0; j < err_vec[i].error_count; j++) {
> -

Random whitespace change?

>  					reg_val = elm_read_reg(info, offset);
>  					err_vec[i].error_loc[j] = reg_val &
>  						ECC_ERROR_LOCATION_MASK;
> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> index bf0a83b..d16465b 100644
> --- a/include/linux/platform_data/elm.h
> +++ b/include/linux/platform_data/elm.h
> @@ -25,6 +25,7 @@ enum bch_ecc {
>  
>  /* ELM support 8 error syndrome process */
>  #define ERROR_VECTOR_MAX		8
> +#define ELM_MAX_DETECTABLE_ERRORS	16

Why 16? Isn't 8 the actual max, with 4K page and 512B sector?

>  
>  #define BCH8_ECC_OOB_BYTES		13
>  #define BCH4_ECC_OOB_BYTES		7

Brian

Patch

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index d1dd6a3..2167384 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -22,8 +22,11 @@ 
 #include <linux/of.h>
 #include <linux/sched.h>
 #include <linux/pm_runtime.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
 #include <linux/platform_data/elm.h>
 
+#define	DRIVER_NAME			"omap-elm"
 #define ELM_SYSCONFIG			0x010
 #define ELM_IRQSTATUS			0x018
 #define ELM_IRQENABLE			0x01c
@@ -82,8 +85,10 @@  struct elm_info {
 	void __iomem *elm_base;
 	struct completion elm_completion;
 	struct list_head list;
+	struct mtd_info *mtd;
 	enum bch_ecc bch_type;
 	struct elm_registers elm_regs;
+	int eccsteps;
 };
 
 static LIST_HEAD(elm_devices);
@@ -103,19 +108,42 @@  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, struct mtd_info *mtd,
+		enum bch_ecc bch_type)
 {
 	u32 reg_val;
-	struct elm_info *info = dev_get_drvdata(dev);
-
+	struct elm_info	 *info;
+	struct nand_chip *chip;
+	if (!dev) {
+		pr_err("%s: ELM device not found\n", DRIVER_NAME);
+		return -ENODEV;
+	}
+	info = dev_get_drvdata(dev);
 	if (!info) {
-		dev_err(dev, "Unable to configure elm - device not probed?\n");
+		pr_err("%s: ELM device data not found\n", DRIVER_NAME);
 		return -ENODEV;
 	}
-
+	if (!mtd) {
+		pr_err("%s: MTD device not found\n", DRIVER_NAME);
+		return -ENODEV;
+	}
+	chip = mtd->priv;
+	/* ELM supports error correction in chunks of 512bytes of data only
+	 * where each 512bytes of data has its own ECC syndrome */
+	if (chip->ecc.size != 512) {
+		pr_err("%s: invalid ecc_size configuration", DRIVER_NAME);
+		return -EINVAL;
+	}
+	if (mtd->writesize > 4096) {
+		pr_err("%s: page-size > 4096 is not supported", DRIVER_NAME);
+		return -EINVAL;
+	}
+	/* ELM eccsteps required to decode complete NAND page */
+	info->mtd	= mtd;
+	info->bch_type	= bch_type;
+	info->eccsteps = mtd->writesize / chip->ecc.size;
 	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;
 
 	return 0;
 }
@@ -152,55 +180,51 @@  static void elm_configure_page_mode(struct elm_info *info, int index,
  * Load syndrome fragment registers with calculated ecc in reverse order.
  */
 static void elm_load_syndrome(struct elm_info *info,
-		struct elm_errorvec *err_vec, u8 *ecc)
+		struct elm_errorvec *err_vec, u8 *ecc_calc)
 {
+	struct nand_chip *chip	= info->mtd->priv;
+	unsigned int eccbytes	= chip->ecc.bytes;
+	u8 *ecc = ecc_calc;
 	int i, offset;
 	u32 val;
 
-	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
-
+	for (i = 0; i < info->eccsteps; i++) {
 		/* Check error reported */
 		if (err_vec[i].error_reported) {
 			elm_configure_page_mode(info, i, true);
-			offset = ELM_SYNDROME_FRAGMENT_0 +
-				SYNDROME_FRAGMENT_REG_SIZE * i;
-
-			/* BCH8 */
-			if (info->bch_type) {
-
-				/* syndrome fragment 0 = ecc[9-12B] */
-				val = cpu_to_be32(*(u32 *) &ecc[9]);
-				elm_write_reg(info, offset, val);
-
-				/* syndrome fragment 1 = ecc[5-8B] */
-				offset += 4;
-				val = cpu_to_be32(*(u32 *) &ecc[5]);
-				elm_write_reg(info, offset, val);
-
-				/* syndrome fragment 2 = ecc[1-4B] */
-				offset += 4;
-				val = cpu_to_be32(*(u32 *) &ecc[1]);
-				elm_write_reg(info, offset, val);
-
-				/* syndrome fragment 3 = ecc[0B] */
-				offset += 4;
-				val = ecc[0];
-				elm_write_reg(info, offset, val);
-			} else {
-				/* syndrome fragment 0 = ecc[20-52b] bits */
-				val = (cpu_to_be32(*(u32 *) &ecc[3]) >> 4) |
-					((ecc[2] & 0xf) << 28);
-				elm_write_reg(info, offset, val);
-
-				/* syndrome fragment 1 = ecc[0-20b] bits */
-				offset += 4;
-				val = cpu_to_be32(*(u32 *) &ecc[0]) >> 12;
-				elm_write_reg(info, offset, val);
+			offset = SYNDROME_FRAGMENT_REG_SIZE * i;
+			ecc = ecc_calc + (i * eccbytes);
+			switch (info->bch_type) {
+			case BCH4_ECC:
+				val =	((*(ecc + 6) >>  4) & 0x0F) |
+					*(ecc +  5) <<  4 | *(ecc +  4) << 12 |
+					*(ecc +  3) << 20 | *(ecc +  2) << 28;
+				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_0 +
+						 offset), cpu_to_le32(val));
+				val =	((*(ecc + 2) >>  4) & 0x0F) |
+					*(ecc +  1) <<  4 | *(ecc +  0) << 12;
+				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_1 +
+						 offset), cpu_to_le32(val));
+				break;
+			case BCH8_ECC:
+				val =	*(ecc + 12) << 0  | *(ecc + 11) <<  8 |
+					*(ecc + 10) << 16 | *(ecc +  9) << 24;
+				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_0 +
+						 offset), cpu_to_le32(val));
+				val =	*(ecc +  8) <<  0 | *(ecc +  7) <<  8 |
+					*(ecc +  6) << 16 | *(ecc +  5) << 24;
+				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_1 +
+						 offset), cpu_to_le32(val));
+				val =	*(ecc +  4) <<  0 | *(ecc +  3) <<  8 |
+					*(ecc +  2) << 16 | *(ecc +  1) << 24;
+				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_2 +
+						 offset), cpu_to_le32(val));
+				val =	*(ecc +  0) <<  0 & 0x000000FF;
+				elm_write_reg(info, (ELM_SYNDROME_FRAGMENT_3 +
+						 offset), cpu_to_le32(val));
+				break;
 			}
 		}
-
-		/* Update ecc pointer with ecc byte size */
-		ecc += info->bch_type ? BCH8_SIZE : BCH4_SIZE;
 	}
 }
 
@@ -223,7 +247,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->eccsteps; i++) {
 		if (err_vec[i].error_reported) {
 			offset = ELM_SYNDROME_FRAGMENT_6 +
 				SYNDROME_FRAGMENT_REG_SIZE * i;
@@ -252,7 +276,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->eccsteps; i++) {
 
 		/* Check error reported */
 		if (err_vec[i].error_reported) {
@@ -263,14 +287,12 @@  static void elm_error_correction(struct elm_info *info,
 			if (reg_val & ECC_CORRECTABLE_MASK) {
 				offset = ELM_ERROR_LOCATION_0 +
 					ERROR_LOCATION_SIZE * i;
-
 				/* Read count of correctable errors */
 				err_vec[i].error_count = reg_val &
 					ECC_NB_ERRORS_MASK;
 
 				/* Update the error locations in error vector */
 				for (j = 0; j < err_vec[i].error_count; j++) {
-
 					reg_val = elm_read_reg(info, offset);
 					err_vec[i].error_loc[j] = reg_val &
 						ECC_ERROR_LOCATION_MASK;
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 3a99e29..037e9e7 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1391,7 +1391,7 @@  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))
+	if (elm_config(info->elm_dev, &info->mtd, bch_type))
 		return -ENODEV;
 	return 0;
 }
diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
index bf0a83b..d16465b 100644
--- a/include/linux/platform_data/elm.h
+++ b/include/linux/platform_data/elm.h
@@ -25,6 +25,7 @@  enum bch_ecc {
 
 /* ELM support 8 error syndrome process */
 #define ERROR_VECTOR_MAX		8
+#define ELM_MAX_DETECTABLE_ERRORS	16
 
 #define BCH8_ECC_OOB_BYTES		13
 #define BCH4_ECC_OOB_BYTES		7
@@ -45,10 +46,11 @@  struct elm_errorvec {
 	bool error_reported;
 	bool error_uncorrectable;
 	int error_count;
-	int error_loc[ERROR_VECTOR_MAX];
+	int error_loc[ELM_MAX_DETECTABLE_ERRORS];
 };
 
 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, struct mtd_info *mtd,
+		enum bch_ecc bch_type);
 #endif /* __ELM_H */