[PATCH 0/4] mtd: nand: OMAP: Add support to use ELM as error correction module

[PATCH 0/4] mtd: nand: OMAP: Add support to use ELM as error correction module

[Philip, Avinash]

Adds support to use ELM as BCH 4 & 8 bit error correction module and
adds support for single shot read_page and write_page functions

Platforms containing the ELM module can be used to correct errors
reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit
support is added.

BCH 4 & 8 bit error detection support is already available in mainline
kernel and works with software error correction.

This series is based on top of [1] &[2]

1. linux-next/master
2. linux-omap-dt/for_3.7/dts_part2

Nand driver tested for BCH 4 & 8 bit error correction per sector.
This being tested by introducing bit errors at multiple sectors inside page.

Philip, Avinash (4):
  mtd: nand: omap2: Update nerrors using ecc.strength
  mtd: devices: elm: Add support for ELM error correction
  ARM: OMAP2: gpmc: Add support for BCH ECC scheme
  mtd: nand: omap2: Add data correction support

 arch/arm/mach-omap2/gpmc.c                   |  120 +++++++-
 arch/arm/plat-omap/include/plat/gpmc.h       |    1 +
 drivers/mtd/devices/Makefile                 |    4 +-
 drivers/mtd/devices/elm.c                    |  446 ++++++++++++++++++++++++++
 drivers/mtd/nand/omap2.c                     |  368 +++++++++++++++++++--
 include/linux/platform_data/elm.h            |   64 ++++
 include/linux/platform_data/mtd-nand-omap2.h |    1 +
 7 files changed, 958 insertions(+), 46 deletions(-)
 create mode 100644 drivers/mtd/devices/elm.c
 create mode 100644 include/linux/platform_data/elm.h

[PATCH 1/4] mtd: nand: omap2: Update nerrors using ecc.strength

[Philip, Avinash]

Update number of errors using nand ecc strength.
Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 5b31386... af511a9... M	drivers/mtd/nand/omap2.c
 drivers/mtd/nand/omap2.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5b31386..af511a9 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -111,6 +111,9 @@
 #define	ECCCLEAR			0x100
 #define	ECC1				0x1
 
+#define BCH8_MAX_ERROR		8	/* upto 8 bit coorectable */
+#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
+
 /* oob info generated runtime depending on ecc algorithm and layout selected */
 static struct nand_ecclayout omap_oobinfo;
 /* Define some generic bad / good block scan pattern which are used
@@ -1034,7 +1037,7 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
 						   mtd);
 	struct nand_chip *chip = mtd->priv;
 
-	nerrors = (info->nand.ecc.bytes == 13) ? 8 : 4;
+	nerrors = info->nand.ecc.strength;
 	dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
 	/*
 	 * Program GPMC to perform correction on one 512-byte sector at a time.
@@ -1129,13 +1132,14 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 						   mtd);
 #ifdef CONFIG_MTD_NAND_OMAP_BCH8
-	const int hw_errors = 8;
+	const int hw_errors = BCH8_MAX_ERROR;
 #else
-	const int hw_errors = 4;
+	const int hw_errors = BCH4_MAX_ERROR;
 #endif
 	info->bch = NULL;
 
-	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ? 8 : 4;
+	max_errors = (ecc_opt == OMAP_ECC_BCH8_CODE_HW) ?
+		BCH8_MAX_ERROR : BCH4_MAX_ERROR;
 	if (max_errors != hw_errors) {
 		pr_err("cannot configure %d-bit BCH ecc, only %d-bit supported",
 		       max_errors, hw_errors);
-- 
1.7.0.4

Re: [PATCH 1/4] mtd: nand: omap2: Update nerrors using ecc.strength

[Peter Korsgaard]

>>>>> Philip, Avinash <avinashphilip@ti.com> writes:

 > Update number of errors using nand ecc strength.
 > Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX

 > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
 > ---
 > :100644 100644 5b31386... af511a9... M	drivers/mtd/nand/omap2.c
 >  drivers/mtd/nand/omap2.c |   12 ++++++++----
 >  1 files changed, 8 insertions(+), 4 deletions(-)

 > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
 > index 5b31386..af511a9 100644
 > --- a/drivers/mtd/nand/omap2.c
 > +++ b/drivers/mtd/nand/omap2.c
 > @@ -111,6 +111,9 @@
 >  #define	ECCCLEAR			0x100
 >  #define	ECC1				0x1
 
 > +#define BCH8_MAX_ERROR		8	/* upto 8 bit coorectable */
 > +#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */

First 'correctable' misspelled.

I don't personally think these defines improve readability very much
compared to to just using 4/8, though.

-- 
Bye, Peter Korsgaard

RE: [PATCH 1/4] mtd: nand: omap2: Update nerrors using ecc.strength

[Philip, Avinash]

On Tue, Oct 16, 2012 at 00:26:40, Peter Korsgaard wrote:
> >>>>> Philip, Avinash <avinashphilip@ti.com> writes:
> 
>  > Update number of errors using nand ecc strength.
>  > Also add macro definitions BCH8_ERROR_MAX & BCH4_ERROR_MAX
> 
>  > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
>  > ---
>  > :100644 100644 5b31386... af511a9... M	drivers/mtd/nand/omap2.c
>  >  drivers/mtd/nand/omap2.c |   12 ++++++++----
>  >  1 files changed, 8 insertions(+), 4 deletions(-)
> 
>  > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>  > index 5b31386..af511a9 100644
>  > --- a/drivers/mtd/nand/omap2.c
>  > +++ b/drivers/mtd/nand/omap2.c
>  > @@ -111,6 +111,9 @@
>  >  #define	ECCCLEAR			0x100
>  >  #define	ECC1				0x1
>  
>  > +#define BCH8_MAX_ERROR		8	/* upto 8 bit coorectable */
>  > +#define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
> 
> First 'correctable' misspelled.

I will correct it

> 
> I don't personally think these defines improve readability very much
> compared to to just using 4/8, though.

Thought of removing magic numbers & provide information also.

Thanks
Avinash

> 
> -- 
> Bye, Peter Korsgaard
> 

[PATCH 2/4] mtd: devices: elm: Add support for ELM error correction

[Philip, Avinash]

Platforms containing the ELM module can be used to correct errors
reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit
support is added.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Rob Landley <rob@landley.net>
---
:000000 100644 0000000... b88ee83... A	Documentation/devicetree/bindings/mtd/elm.txt
:100644 100644 395733a... 0f6a94b... M	drivers/mtd/devices/Makefile
:000000 100644 0000000... 802b572... A	drivers/mtd/devices/elm.c
:000000 100644 0000000... eb53163... A	include/linux/platform_data/elm.h
 Documentation/devicetree/bindings/mtd/elm.txt |   18 +
 drivers/mtd/devices/Makefile                  |    4 +-
 drivers/mtd/devices/elm.c                     |  440 +++++++++++++++++++++++++
 include/linux/platform_data/elm.h             |   60 ++++
 4 files changed, 521 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/elm.txt b/Documentation/devicetree/bindings/mtd/elm.txt
new file mode 100644
index 0000000..b88ee83
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/elm.txt
@@ -0,0 +1,18 @@
+Error location module
+
+Required properties:
+- compatible: Must be "ti,elm"
+- reg: physical base address and size of the registers map.
+- interrupts: Interrupt number for the elm.
+- interrupt-parent: The parent interrupt controller
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the elm
+
+Example:
+elm: elm@0 {
+	compatible	= "ti,elm";
+	reg = <0x48080000 0x2000>;
+	interrupt-parent = <&intc>;
+	interrupts = <4>;
+};
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index 395733a..0f6a94b 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
 obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
 obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
 obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
+obj-$(CONFIG_MTD_NAND_OMAP2)	+= elm.o
 obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 
-CFLAGS_docg3.o			+= -I$(src)
\ No newline at end of file
+
+CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
new file mode 100644
index 0000000..802b572
--- /dev/null
+++ b/drivers/mtd/devices/elm.c
@@ -0,0 +1,440 @@
+/*
+ * Error Location Module
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_data/elm.h>
+
+#define ELM_IRQSTATUS			0x018
+#define ELM_IRQENABLE			0x01c
+#define ELM_LOCATION_CONFIG		0x020
+#define ELM_PAGE_CTRL			0x080
+#define ELM_SYNDROME_FRAGMENT_0		0x400
+#define ELM_SYNDROME_FRAGMENT_6		0x418
+#define ELM_LOCATION_STATUS		0x800
+#define ELM_ERROR_LOCATION_0		0x880
+
+/* ELM Interrupt Status Register */
+#define INTR_STATUS_PAGE_VALID		BIT(8)
+
+/* ELM Interrupt Enable Register */
+#define INTR_EN_PAGE_MASK		BIT(8)
+
+/* ELM Location Configuration Register */
+#define ECC_BCH_LEVEL_MASK		0x3
+
+/* ELM syndrome */
+#define ELM_SYNDROME_VALID		BIT(16)
+
+/* ELM_LOCATION_STATUS Register */
+#define ECC_CORRECTABLE_MASK		BIT(8)
+#define ECC_NB_ERRORS_MASK		0x1f
+
+/* ELM_ERROR_LOCATION_0-15 Registers */
+#define ECC_ERROR_LOCATION_MASK		0x1fff
+
+#define ELM_ECC_SIZE			0x7ff
+
+#define SYNDROME_FRAGMENT_REG_SIZE	0x40
+#define ERROR_LOCATION_SIZE		0x100
+#define MAX_BCH_ELM_ERROR		16
+#define ELM_FRAGMENT_REG		7
+
+typedef u32 syn_t[ELM_FRAGMENT_REG];
+typedef u32 elm_error_t[MAX_BCH_ELM_ERROR];
+
+struct elm_info {
+	struct device *dev;
+	void __iomem *elm_base;
+	struct completion elm_completion;
+	struct list_head list;
+	enum bch_ecc bch_type;
+};
+
+static LIST_HEAD(elm_devices);
+
+static void elm_write_reg(void *offset, u32 val)
+{
+	writel(val, offset);
+}
+
+static u32 elm_read_reg(void *offset)
+{
+	return readl(offset);
+}
+
+/**
+ * elm_config - Configure ELM module
+ * @info:	elm info
+ */
+static void elm_config(struct elm_info *info)
+{
+	u32 reg_val;
+
+	reg_val = (info->bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
+	elm_write_reg(info->elm_base + ELM_LOCATION_CONFIG, reg_val);
+}
+
+/**
+ * elm_configure_page_mode - Enable/Disable page mode
+ * @info:	elm info
+ * @index:	index number of syndrome fragment vector
+ * @enable:	enable/disable flag for page mode
+ *
+ * Enable page mode for syndrome fragment index
+ */
+static void elm_configure_page_mode(struct elm_info *info, int index,
+		bool enable)
+{
+	u32 reg_val;
+
+	reg_val = elm_read_reg(info->elm_base + ELM_PAGE_CTRL);
+	if (enable)
+		reg_val |= BIT(index);	/* enable page mode */
+	else
+		reg_val &= ~BIT(index);	/* disable page mode */
+
+	elm_write_reg(info->elm_base + ELM_PAGE_CTRL, reg_val);
+}
+
+static void rearrange_bch4(u8 *syndrome)
+{
+	/*
+	 * BCH4 has 52 bit used for ecc, but OOB stored with
+	 * nibble 0 appended, removes appended 0 nibble
+	 */
+	u64 *dst = (u64 *)syndrome;
+	*dst >>= 4;
+}
+
+/**
+ * elm_reverse_eccdata - Reverse ecc data
+ * @info:	elm info
+ * @ecc_data:	buffer for calculated ecc
+ * @syndrome:	buffer for syndrome polynomial
+ *
+ * ecc_data has to be reversed for syndrome vector
+ */
+static void elm_reverse_eccdata(struct elm_info *info, u8 *ecc_data,
+		u8 *syndrome)
+{
+	int i;
+	int bch_size = info->bch_type ? BCH8_ECC_OOB_BYTES : BCH4_SIZE;
+
+	for (i = 0; i < bch_size; i++)
+		syndrome[bch_size - 1 - i] = ecc_data[i];
+
+	/*
+	 * syndrome polynomial to be rearranged for BCH 4 ecc scheme as 52
+	 * bits being used and 4 bits being appended in syndrome vector
+	 */
+	if (info->bch_type == BCH4_ECC)
+		rearrange_bch4(syndrome);
+}
+
+/**
+ * elm_load_syndrome - Load ELM syndrome reg
+ * @info:	elm info
+ * @index:	syndrome fragment index
+ * @syndrome:	Syndrome polynomial
+ *
+ * Load syndrome fragment registers with syndrome polynomial
+ */
+static void elm_load_syndrome(struct elm_info *info, int index, u8 *syndrome)
+{
+	int i;
+	int max = info->bch_type ? BCH8_SYNDROME_SIZE : BCH4_SYNDROME_SIZE;
+	void *syn_frag_base = info->elm_base + ELM_SYNDROME_FRAGMENT_0;
+	syn_t *syn_fragment;
+	u32 reg_val;
+
+	elm_configure_page_mode(info, index, true);
+	syn_fragment = syn_frag_base + SYNDROME_FRAGMENT_REG_SIZE * index;
+
+	for (i = 0; i < max; i++) {
+		reg_val = syndrome[0] | syndrome[1] << 8 | syndrome[2] << 16 |
+			syndrome[3] << 24;
+		elm_write_reg(*syn_fragment + i, reg_val);
+		/* Update syndrome polynomial pointer */
+		syndrome += 4;
+	}
+}
+
+/**
+ * elm_start_processing - start elm syndrome processing
+ * @info:	elm info
+ * @err_vec:	elm error vectors
+ *
+ * Set syndrome valid bit for syndrome fragment registers for which
+ * elm syndrome fragment registers are loaded. This enables elm module
+ * to start processing syndrome vectors.
+ */
+static void elm_start_processing(struct elm_info *info,
+		struct elm_errorvec *err_vec)
+{
+	int i;
+	void *offset;
+	u32 reg_val;
+
+	/*
+	 * Set syndrome vector valid so that ELM module will process it for
+	 * vectors error is reported
+	 */
+	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
+		if (err_vec[i].error_reported) {
+			offset = info->elm_base + ELM_SYNDROME_FRAGMENT_6 + i *
+				SYNDROME_FRAGMENT_REG_SIZE;
+			reg_val = elm_read_reg(offset);
+			reg_val |= ELM_SYNDROME_VALID;
+			elm_write_reg(offset, reg_val);
+		}
+	}
+}
+
+/**
+ * elm_error_correction - locate correctable error position
+ * @info:	elm info
+ * @err_vec:	elm error vectors
+ *
+ * On completion of processing by elm module, error location status
+ * register updated with correctable/uncorrectable error information.
+ * In case of correctable errors, number of errors located from
+ * elm location status register & read the these many positions from
+ * elm error location register.
+ */
+static void elm_error_correction(struct elm_info *info,
+		struct elm_errorvec *err_vec)
+{
+	int i, j, errors = 0;
+	void *err_loc_base = info->elm_base + ELM_ERROR_LOCATION_0;
+	elm_error_t *err_loc;
+	void *offset;
+	u32 reg_val;
+
+	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
+		/* check error reported */
+		if (err_vec[i].error_reported) {
+			offset = info->elm_base + ELM_LOCATION_STATUS +
+				i * ERROR_LOCATION_SIZE;
+			reg_val = elm_read_reg(offset);
+			/* check correctable error or not */
+			if (reg_val & ECC_CORRECTABLE_MASK) {
+				err_loc = err_loc_base +
+					i * ERROR_LOCATION_SIZE;
+				/* 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(*err_loc + j);
+					err_vec[i].error_loc[j] = reg_val &
+						ECC_ERROR_LOCATION_MASK;
+				}
+
+				errors += err_vec[i].error_count;
+			} else {
+				err_vec[i].error_uncorrectable++;
+			}
+
+			/* clearing interrupts for processed error vectors */
+			elm_write_reg(info->elm_base + ELM_IRQSTATUS, BIT(i));
+
+			/* disable page mode */
+			elm_configure_page_mode(info, i, false);
+		}
+	}
+
+	return;
+}
+
+/**
+ * elm_decode_bch_error_page - Locate error position
+ * @info:	elm info
+ * @ecc_calc:	calculated ECC bytes from GPMC
+ * @err_vec:	elm error vectors
+ *
+ * Called with one or greater reported and is vectors with error reported
+ * is updated in err_vec[].error_reported
+ */
+void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
+		struct elm_errorvec *err_vec)
+{
+	int i;
+	u8 syndrome[BCH_MAX_ECC_BYTES_PER_SECTOR] = {0}, *syn_p;
+	struct elm_info *info = dev_get_drvdata(dev);
+	u32 reg_val;
+
+	/* enable page mode interrupt */
+	reg_val = elm_read_reg(info->elm_base + ELM_IRQSTATUS);
+	elm_write_reg(info->elm_base + ELM_IRQSTATUS,
+			reg_val & INTR_STATUS_PAGE_VALID);
+	elm_write_reg(info->elm_base + ELM_IRQENABLE, INTR_EN_PAGE_MASK);
+
+	syn_p = syndrome;
+	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
+		if (err_vec[i].error_reported) {
+			elm_reverse_eccdata(info, ecc_calc, syn_p);
+			elm_load_syndrome(info, i, syn_p);
+		}
+
+		ecc_calc += info->bch_type ? BCH8_SIZE : BCH4_SIZE;
+		syn_p += OOB_SECTOR_SIZE;
+	}
+
+	elm_start_processing(info, err_vec);
+
+	/*
+	 * In case of error reported, wait for ELM module to finish
+	 * locating error correction
+	 */
+	wait_for_completion(&info->elm_completion);
+
+	/* disable page mode interrupt */
+	reg_val = elm_read_reg(info->elm_base + ELM_IRQENABLE);
+	elm_write_reg(info->elm_base + ELM_IRQENABLE,
+			reg_val & ~INTR_EN_PAGE_MASK);
+	elm_error_correction(info, err_vec);
+}
+EXPORT_SYMBOL(elm_decode_bch_error_page);
+
+static irqreturn_t elm_isr(int this_irq, void *dev_id)
+{
+	u32 reg_val;
+	struct elm_info *info = dev_id;
+
+	reg_val = elm_read_reg(info->elm_base + ELM_IRQSTATUS);
+
+	/* all error vectors processed */
+	if (reg_val & INTR_STATUS_PAGE_VALID) {
+		elm_write_reg(info->elm_base + ELM_IRQSTATUS,
+				reg_val & INTR_STATUS_PAGE_VALID);
+		complete(&info->elm_completion);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+struct device *elm_request(enum bch_ecc bch_type)
+{
+	struct elm_info *info;
+
+	list_for_each_entry(info, &elm_devices, list) {
+		if (info && info->dev) {
+				info->bch_type = bch_type;
+				elm_config(info);
+				return info->dev;
+		}
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(elm_request);
+
+static int __devinit elm_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct resource *res, *irq;
+	struct elm_info *info;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	info->dev = &pdev->dev;
+
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "no irq resource defined\n");
+		return -ENODEV;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no memory resource defined\n");
+		return -ENODEV;
+	}
+
+
+	info->elm_base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!info->elm_base)
+		return -EADDRNOTAVAIL;
+
+
+	ret = devm_request_irq(&pdev->dev, irq->start, elm_isr, 0,
+			pdev->name, info);
+	if (ret) {
+		dev_err(&pdev->dev, "failure requesting irq %i\n", irq->start);
+		return ret;
+	}
+
+	pm_runtime_enable(&pdev->dev);
+	if (pm_runtime_get_sync(&pdev->dev)) {
+		ret = -EINVAL;
+		pm_runtime_disable(&pdev->dev);
+		dev_err(&pdev->dev, "can't enable clock\n");
+		return ret;
+	}
+
+	init_completion(&info->elm_completion);
+	INIT_LIST_HEAD(&info->list);
+	list_add(&info->list, &elm_devices);
+	platform_set_drvdata(pdev, info);
+	return ret;
+}
+
+static int __devexit elm_remove(struct platform_device *pdev)
+{
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+
+#ifdef CONFIG_OF
+static const struct of_device_id elm_of_match[] = {
+	{ .compatible = "ti,elm" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, elm_of_match);
+#endif
+
+static struct platform_driver elm_driver = {
+	.driver		= {
+		.name	= "elm",
+		.of_match_table = of_match_ptr(elm_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= elm_probe,
+	.remove		= __devexit_p(elm_remove),
+};
+
+module_platform_driver(elm_driver);
+
+MODULE_DESCRIPTION("ELM driver for BCH error correction");
+MODULE_AUTHOR("Texas Instruments");
+MODULE_ALIAS("platform: elm");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
new file mode 100644
index 0000000..eb53163
--- /dev/null
+++ b/include/linux/platform_data/elm.h
@@ -0,0 +1,60 @@
+/*
+ * BCH Error Location Module
+ *
+ * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __ELM_H
+#define __ELM_H
+
+enum bch_ecc {
+	BCH4_ECC = 0,
+	BCH8_ECC,
+	BCH16_ECC,
+};
+
+/* ELM support 8 error syndrome process */
+#define ERROR_VECTOR_MAX		8
+#define OOB_SECTOR_SIZE			16
+
+#define BCH_MAX_ECC_BYTES_PER_SECTOR	(OOB_SECTOR_SIZE * ERROR_VECTOR_MAX)
+
+#define BCH8_ECC_OOB_BYTES		13
+/* RBL requires 14 byte even though BCH8 uses only 13 byte */
+#define BCH8_SIZE			(BCH8_ECC_OOB_BYTES + 1)
+#define BCH4_SIZE			7
+
+#define	BCH8_SYNDROME_SIZE		4	/* 13 bytes of ecc */
+#define	BCH4_SYNDROME_SIZE		2	/* 7 bytes of ecc */
+
+/**
+ * struct elm_errorvec - error vector for elm
+ * @error_reported:		set true for vectors error is reported
+ *
+ * @error_count:		number of correctable errors in the sector
+ * @error_uncorrectable:	number of uncorrectable errors
+ * @error_loc:			buffer for error location
+ *
+ */
+struct elm_errorvec {
+	bool error_reported;
+	int error_count;
+	int error_uncorrectable;
+	int error_loc[ERROR_VECTOR_MAX];
+};
+
+void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
+		struct elm_errorvec *err_vec);
+struct device *elm_request(enum bch_ecc bch_type);
+#endif /* __ELM_H */
-- 
1.7.0.4

Re: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction

[Peter Meerwald]

some minor nitpicks below

> Platforms containing the ELM module can be used to correct errors
> reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit
> support is added.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Rob Landley <rob@landley.net>
> ---
> :000000 100644 0000000... b88ee83... A	Documentation/devicetree/bindings/mtd/elm.txt
> :100644 100644 395733a... 0f6a94b... M	drivers/mtd/devices/Makefile
> :000000 100644 0000000... 802b572... A	drivers/mtd/devices/elm.c
> :000000 100644 0000000... eb53163... A	include/linux/platform_data/elm.h
>  Documentation/devicetree/bindings/mtd/elm.txt |   18 +
>  drivers/mtd/devices/Makefile                  |    4 +-
>  drivers/mtd/devices/elm.c                     |  440 +++++++++++++++++++++++++
>  include/linux/platform_data/elm.h             |   60 ++++
>  4 files changed, 521 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/elm.txt b/Documentation/devicetree/bindings/mtd/elm.txt
> new file mode 100644
> index 0000000..b88ee83
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/elm.txt
> @@ -0,0 +1,18 @@
> +Error location module
> +
> +Required properties:
> +- compatible: Must be "ti,elm"
> +- reg: physical base address and size of the registers map.
> +- interrupts: Interrupt number for the elm.
> +- interrupt-parent: The parent interrupt controller
> +
> +Optional properties:
> +- ti,hwmods: Name of the hwmod associated to the elm
> +
> +Example:
> +elm: elm@0 {
> +	compatible	= "ti,elm";
> +	reg = <0x48080000 0x2000>;
> +	interrupt-parent = <&intc>;
> +	interrupts = <4>;
> +};
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index 395733a..0f6a94b 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
>  obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
>  obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
>  obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
> +obj-$(CONFIG_MTD_NAND_OMAP2)	+= elm.o
>  obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
>  obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
>  obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
>  
> -CFLAGS_docg3.o			+= -I$(src)
> \ No newline at end of file
> +
> +CFLAGS_docg3.o			+= -I$(src)
> diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
> new file mode 100644
> index 0000000..802b572
> --- /dev/null
> +++ b/drivers/mtd/devices/elm.c
> @@ -0,0 +1,440 @@
> +/*
> + * Error Location Module
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/platform_data/elm.h>
> +
> +#define ELM_IRQSTATUS			0x018
> +#define ELM_IRQENABLE			0x01c
> +#define ELM_LOCATION_CONFIG		0x020
> +#define ELM_PAGE_CTRL			0x080
> +#define ELM_SYNDROME_FRAGMENT_0		0x400
> +#define ELM_SYNDROME_FRAGMENT_6		0x418
> +#define ELM_LOCATION_STATUS		0x800
> +#define ELM_ERROR_LOCATION_0		0x880
> +
> +/* ELM Interrupt Status Register */
> +#define INTR_STATUS_PAGE_VALID		BIT(8)
> +
> +/* ELM Interrupt Enable Register */
> +#define INTR_EN_PAGE_MASK		BIT(8)
> +
> +/* ELM Location Configuration Register */
> +#define ECC_BCH_LEVEL_MASK		0x3
> +
> +/* ELM syndrome */
> +#define ELM_SYNDROME_VALID		BIT(16)
> +
> +/* ELM_LOCATION_STATUS Register */
> +#define ECC_CORRECTABLE_MASK		BIT(8)
> +#define ECC_NB_ERRORS_MASK		0x1f
> +
> +/* ELM_ERROR_LOCATION_0-15 Registers */
> +#define ECC_ERROR_LOCATION_MASK		0x1fff
> +
> +#define ELM_ECC_SIZE			0x7ff
> +
> +#define SYNDROME_FRAGMENT_REG_SIZE	0x40
> +#define ERROR_LOCATION_SIZE		0x100
> +#define MAX_BCH_ELM_ERROR		16
> +#define ELM_FRAGMENT_REG		7
> +
> +typedef u32 syn_t[ELM_FRAGMENT_REG];
> +typedef u32 elm_error_t[MAX_BCH_ELM_ERROR];
> +
> +struct elm_info {
> +	struct device *dev;
> +	void __iomem *elm_base;
> +	struct completion elm_completion;
> +	struct list_head list;
> +	enum bch_ecc bch_type;
> +};
> +
> +static LIST_HEAD(elm_devices);
> +
> +static void elm_write_reg(void *offset, u32 val)
> +{
> +	writel(val, offset);
> +}
> +
> +static u32 elm_read_reg(void *offset)
> +{
> +	return readl(offset);
> +}
> +
> +/**
> + * elm_config - Configure ELM module
> + * @info:	elm info
> + */
> +static void elm_config(struct elm_info *info)
> +{
> +	u32 reg_val;
> +
> +	reg_val = (info->bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
> +	elm_write_reg(info->elm_base + ELM_LOCATION_CONFIG, reg_val);
> +}
> +
> +/**
> + * elm_configure_page_mode - Enable/Disable page mode
> + * @info:	elm info
> + * @index:	index number of syndrome fragment vector
> + * @enable:	enable/disable flag for page mode
> + *
> + * Enable page mode for syndrome fragment index
> + */
> +static void elm_configure_page_mode(struct elm_info *info, int index,
> +		bool enable)
> +{
> +	u32 reg_val;
> +
> +	reg_val = elm_read_reg(info->elm_base + ELM_PAGE_CTRL);
> +	if (enable)
> +		reg_val |= BIT(index);	/* enable page mode */
> +	else
> +		reg_val &= ~BIT(index);	/* disable page mode */
> +
> +	elm_write_reg(info->elm_base + ELM_PAGE_CTRL, reg_val);
> +}
> +
> +static void rearrange_bch4(u8 *syndrome)
> +{
> +	/*
> +	 * BCH4 has 52 bit used for ecc, but OOB stored with
> +	 * nibble 0 appended, removes appended 0 nibble
> +	 */
> +	u64 *dst = (u64 *)syndrome;
> +	*dst >>= 4;
> +}
> +
> +/**
> + * elm_reverse_eccdata - Reverse ecc data
> + * @info:	elm info
> + * @ecc_data:	buffer for calculated ecc
> + * @syndrome:	buffer for syndrome polynomial
> + *
> + * ecc_data has to be reversed for syndrome vector
> + */
> +static void elm_reverse_eccdata(struct elm_info *info, u8 *ecc_data,
> +		u8 *syndrome)
> +{
> +	int i;
> +	int bch_size = info->bch_type ? BCH8_ECC_OOB_BYTES : BCH4_SIZE;
> +
> +	for (i = 0; i < bch_size; i++)
> +		syndrome[bch_size - 1 - i] = ecc_data[i];
> +
> +	/*
> +	 * syndrome polynomial to be rearranged for BCH 4 ecc scheme as 52
> +	 * bits being used and 4 bits being appended in syndrome vector
> +	 */
> +	if (info->bch_type == BCH4_ECC)
> +		rearrange_bch4(syndrome);
> +}
> +
> +/**
> + * elm_load_syndrome - Load ELM syndrome reg
> + * @info:	elm info
> + * @index:	syndrome fragment index
> + * @syndrome:	Syndrome polynomial
> + *
> + * Load syndrome fragment registers with syndrome polynomial
> + */
> +static void elm_load_syndrome(struct elm_info *info, int index, u8 *syndrome)
> +{
> +	int i;
> +	int max = info->bch_type ? BCH8_SYNDROME_SIZE : BCH4_SYNDROME_SIZE;
> +	void *syn_frag_base = info->elm_base + ELM_SYNDROME_FRAGMENT_0;
> +	syn_t *syn_fragment;
> +	u32 reg_val;
> +
> +	elm_configure_page_mode(info, index, true);
> +	syn_fragment = syn_frag_base + SYNDROME_FRAGMENT_REG_SIZE * index;
> +
> +	for (i = 0; i < max; i++) {
> +		reg_val = syndrome[0] | syndrome[1] << 8 | syndrome[2] << 16 |
> +			syndrome[3] << 24;
> +		elm_write_reg(*syn_fragment + i, reg_val);
> +		/* Update syndrome polynomial pointer */
> +		syndrome += 4;
> +	}
> +}
> +
> +/**
> + * elm_start_processing - start elm syndrome processing
> + * @info:	elm info
> + * @err_vec:	elm error vectors
> + *
> + * Set syndrome valid bit for syndrome fragment registers for which
> + * elm syndrome fragment registers are loaded. This enables elm module
> + * to start processing syndrome vectors.
> + */
> +static void elm_start_processing(struct elm_info *info,
> +		struct elm_errorvec *err_vec)
> +{
> +	int i;
> +	void *offset;
> +	u32 reg_val;
> +
> +	/*
> +	 * Set syndrome vector valid so that ELM module will process it for
> +	 * vectors error is reported
> +	 */
> +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
> +		if (err_vec[i].error_reported) {
> +			offset = info->elm_base + ELM_SYNDROME_FRAGMENT_6 + i *
> +				SYNDROME_FRAGMENT_REG_SIZE;
> +			reg_val = elm_read_reg(offset);
> +			reg_val |= ELM_SYNDROME_VALID;
> +			elm_write_reg(offset, reg_val);
> +		}
> +	}
> +}
> +
> +/**
> + * elm_error_correction - locate correctable error position
> + * @info:	elm info
> + * @err_vec:	elm error vectors
> + *
> + * On completion of processing by elm module, error location status
> + * register updated with correctable/uncorrectable error information.
> + * In case of correctable errors, number of errors located from
> + * elm location status register & read the these many positions from

should probably be: "... & read the positions from ..."?

> + * elm error location register.
> + */
> +static void elm_error_correction(struct elm_info *info,
> +		struct elm_errorvec *err_vec)
> +{
> +	int i, j, errors = 0;
> +	void *err_loc_base = info->elm_base + ELM_ERROR_LOCATION_0;
> +	elm_error_t *err_loc;
> +	void *offset;
> +	u32 reg_val;
> +
> +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
> +		/* check error reported */
> +		if (err_vec[i].error_reported) {
> +			offset = info->elm_base + ELM_LOCATION_STATUS +
> +				i * ERROR_LOCATION_SIZE;
> +			reg_val = elm_read_reg(offset);
> +			/* check correctable error or not */
> +			if (reg_val & ECC_CORRECTABLE_MASK) {
> +				err_loc = err_loc_base +
> +					i * ERROR_LOCATION_SIZE;
> +				/* 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(*err_loc + j);
> +					err_vec[i].error_loc[j] = reg_val &
> +						ECC_ERROR_LOCATION_MASK;
> +				}
> +
> +				errors += err_vec[i].error_count;
> +			} else {
> +				err_vec[i].error_uncorrectable++;
> +			}

extra braces above

> +
> +			/* clearing interrupts for processed error vectors */
> +			elm_write_reg(info->elm_base + ELM_IRQSTATUS, BIT(i));
> +
> +			/* disable page mode */
> +			elm_configure_page_mode(info, i, false);
> +		}
> +	}
> +
> +	return;
> +}
> +
> +/**
> + * elm_decode_bch_error_page - Locate error position
> + * @info:	elm info

should be dev, not info

> + * @ecc_calc:	calculated ECC bytes from GPMC
> + * @err_vec:	elm error vectors
> + *
> + * Called with one or greater reported and is vectors with error reported
> + * is updated in err_vec[].error_reported
> + */

I do not understand the statement "Called with one ..."

> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> +		struct elm_errorvec *err_vec)
> +{
> +	int i;
> +	u8 syndrome[BCH_MAX_ECC_BYTES_PER_SECTOR] = {0}, *syn_p;
> +	struct elm_info *info = dev_get_drvdata(dev);
> +	u32 reg_val;
> +
> +	/* enable page mode interrupt */
> +	reg_val = elm_read_reg(info->elm_base + ELM_IRQSTATUS);
> +	elm_write_reg(info->elm_base + ELM_IRQSTATUS,
> +			reg_val & INTR_STATUS_PAGE_VALID);
> +	elm_write_reg(info->elm_base + ELM_IRQENABLE, INTR_EN_PAGE_MASK);
> +
> +	syn_p = syndrome;
> +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
> +		if (err_vec[i].error_reported) {
> +			elm_reverse_eccdata(info, ecc_calc, syn_p);
> +			elm_load_syndrome(info, i, syn_p);
> +		}
> +
> +		ecc_calc += info->bch_type ? BCH8_SIZE : BCH4_SIZE;
> +		syn_p += OOB_SECTOR_SIZE;
> +	}
> +
> +	elm_start_processing(info, err_vec);
> +
> +	/*
> +	 * In case of error reported, wait for ELM module to finish
> +	 * locating error correction
> +	 */
> +	wait_for_completion(&info->elm_completion);
> +
> +	/* disable page mode interrupt */
> +	reg_val = elm_read_reg(info->elm_base + ELM_IRQENABLE);
> +	elm_write_reg(info->elm_base + ELM_IRQENABLE,
> +			reg_val & ~INTR_EN_PAGE_MASK);
> +	elm_error_correction(info, err_vec);
> +}
> +EXPORT_SYMBOL(elm_decode_bch_error_page);
> +
> +static irqreturn_t elm_isr(int this_irq, void *dev_id)
> +{
> +	u32 reg_val;
> +	struct elm_info *info = dev_id;
> +
> +	reg_val = elm_read_reg(info->elm_base + ELM_IRQSTATUS);
> +
> +	/* all error vectors processed */
> +	if (reg_val & INTR_STATUS_PAGE_VALID) {
> +		elm_write_reg(info->elm_base + ELM_IRQSTATUS,
> +				reg_val & INTR_STATUS_PAGE_VALID);
> +		complete(&info->elm_completion);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +struct device *elm_request(enum bch_ecc bch_type)
> +{
> +	struct elm_info *info;
> +
> +	list_for_each_entry(info, &elm_devices, list) {
> +		if (info && info->dev) {
> +				info->bch_type = bch_type;
> +				elm_config(info);
> +				return info->dev;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(elm_request);
> +
> +static int __devinit elm_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +	struct resource *res, *irq;
> +	struct elm_info *info;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	info->dev = &pdev->dev;
> +
> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!irq) {
> +		dev_err(&pdev->dev, "no irq resource defined\n");
> +		return -ENODEV;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		return -ENODEV;
> +	}
> +
> +
> +	info->elm_base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!info->elm_base)
> +		return -EADDRNOTAVAIL;
> +
> +
> +	ret = devm_request_irq(&pdev->dev, irq->start, elm_isr, 0,
> +			pdev->name, info);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failure requesting irq %i\n", irq->start);
> +		return ret;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	if (pm_runtime_get_sync(&pdev->dev)) {
> +		ret = -EINVAL;
> +		pm_runtime_disable(&pdev->dev);
> +		dev_err(&pdev->dev, "can't enable clock\n");
> +		return ret;
> +	}
> +
> +	init_completion(&info->elm_completion);
> +	INIT_LIST_HEAD(&info->list);
> +	list_add(&info->list, &elm_devices);
> +	platform_set_drvdata(pdev, info);
> +	return ret;
> +}
> +
> +static int __devexit elm_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id elm_of_match[] = {
> +	{ .compatible = "ti,elm" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, elm_of_match);
> +#endif
> +
> +static struct platform_driver elm_driver = {
> +	.driver		= {
> +		.name	= "elm",
> +		.of_match_table = of_match_ptr(elm_of_match),
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= elm_probe,
> +	.remove		= __devexit_p(elm_remove),
> +};
> +
> +module_platform_driver(elm_driver);
> +
> +MODULE_DESCRIPTION("ELM driver for BCH error correction");
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_ALIAS("platform: elm");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> new file mode 100644
> index 0000000..eb53163
> --- /dev/null
> +++ b/include/linux/platform_data/elm.h
> @@ -0,0 +1,60 @@
> +/*
> + * BCH Error Location Module
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __ELM_H
> +#define __ELM_H
> +
> +enum bch_ecc {
> +	BCH4_ECC = 0,
> +	BCH8_ECC,
> +	BCH16_ECC,
> +};
> +
> +/* ELM support 8 error syndrome process */
> +#define ERROR_VECTOR_MAX		8
> +#define OOB_SECTOR_SIZE			16
> +
> +#define BCH_MAX_ECC_BYTES_PER_SECTOR	(OOB_SECTOR_SIZE * ERROR_VECTOR_MAX)
> +
> +#define BCH8_ECC_OOB_BYTES		13
> +/* RBL requires 14 byte even though BCH8 uses only 13 byte */

not sure what RBL is?

> +#define BCH8_SIZE			(BCH8_ECC_OOB_BYTES + 1)
> +#define BCH4_SIZE			7
> +
> +#define	BCH8_SYNDROME_SIZE		4	/* 13 bytes of ecc */
> +#define	BCH4_SYNDROME_SIZE		2	/* 7 bytes of ecc */
> +
> +/**
> + * struct elm_errorvec - error vector for elm
> + * @error_reported:		set true for vectors error is reported
> + *
> + * @error_count:		number of correctable errors in the sector
> + * @error_uncorrectable:	number of uncorrectable errors
> + * @error_loc:			buffer for error location
> + *
> + */
> +struct elm_errorvec {
> +	bool error_reported;
> +	int error_count;
> +	int error_uncorrectable;
> +	int error_loc[ERROR_VECTOR_MAX];
> +};
> +
> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> +		struct elm_errorvec *err_vec);
> +struct device *elm_request(enum bch_ecc bch_type);
> +#endif /* __ELM_H */
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

RE: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction

[Philip, Avinash]

On Wed, Oct 03, 2012 at 20:40:46, Peter Meerwald wrote:
> 
> some minor nitpicks below
> 
> > + *
> > + * On completion of processing by elm module, error location status
> > + * register updated with correctable/uncorrectable error information.
> > + * In case of correctable errors, number of errors located from
> > + * elm location status register & read the these many positions from
> 
> should probably be: "... & read the positions from ..."?

Ok I will correct it.

> 
> > + * elm error location register.
> > + */
> > +static void elm_error_correction(struct elm_info *info,
> > +		struct elm_errorvec *err_vec)
> > +{
> > +	int i, j, errors = 0;
> > +	void *err_loc_base = info->elm_base + ELM_ERROR_LOCATION_0;
> > +	elm_error_t *err_loc;
> > +	void *offset;
> > +	u32 reg_val;
> > +
> > +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
> > +		/* check error reported */
> > +		if (err_vec[i].error_reported) {
> > +			offset = info->elm_base + ELM_LOCATION_STATUS +
> > +				i * ERROR_LOCATION_SIZE;
> > +			reg_val = elm_read_reg(offset);
> > +			/* check correctable error or not */
> > +			if (reg_val & ECC_CORRECTABLE_MASK) {
> > +				err_loc = err_loc_base +
> > +					i * ERROR_LOCATION_SIZE;
> > +				/* 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(*err_loc + j);
> > +					err_vec[i].error_loc[j] = reg_val &
> > +						ECC_ERROR_LOCATION_MASK;
> > +				}
> > +
> > +				errors += err_vec[i].error_count;
> > +			} else {
> > +				err_vec[i].error_uncorrectable++;
> > +			}
> 
> extra braces above

As per coding style
This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

> 
> > +
> > +			/* clearing interrupts for processed error vectors */
> > +			elm_write_reg(info->elm_base + ELM_IRQSTATUS, BIT(i));
> > +
> > +			/* disable page mode */
> > +			elm_configure_page_mode(info, i, false);
> > +		}
> > +	}
> > +
> > +	return;
> > +}
> > +
> > +/**
> > + * elm_decode_bch_error_page - Locate error position
> > + * @info:	elm info
> 
> should be dev, not info

Ok I will correct it.

> 
> > + * @ecc_calc:	calculated ECC bytes from GPMC
> > + * @err_vec:	elm error vectors
> > + *
> > + * Called with one or greater reported and is vectors with error reported
> > + * is updated in err_vec[].error_reported
> > + */
> 
> I do not understand the statement "Called with one ..."

elm_decode_bch_error_page() API will called from nand driver if and only if
errors are present while reading page. Errors can be reported in one or
multiple error vectors.

I will reword it as.

Called with one or more error reported vectors & vectors with error reported
is updated in err_vec[].error_reported

> 
[snip]
> > +
> > +#define BCH8_ECC_OOB_BYTES		13
> > +/* RBL requires 14 byte even though BCH8 uses only 13 byte */
> 
> not sure what RBL is?

RBL stands for Rom boot Loader

> 
> > +#define BCH8_SIZE			(BCH8_ECC_OOB_BYTES + 1)
> > +#define BCH4_SIZE			7
> > +
> > +#define	BCH8_SYNDROME_SIZE		4	/* 13 bytes of ecc */
> > +#define	BCH4_SYNDROME_SIZE		2	/* 7 bytes of ecc */
> > +
> > +/**
> > + * struct elm_errorvec - error vector for elm
> > + * @error_reported:		set true for vectors error is reported
> > + *
> > + * @error_count:		number of correctable errors in the sector
> > + * @error_uncorrectable:	number of uncorrectable errors
> > + * @error_loc:			buffer for error location
> > + *
> > + */
> > +struct elm_errorvec {
> > +	bool error_reported;
> > +	int error_count;
> > +	int error_uncorrectable;
> > +	int error_loc[ERROR_VECTOR_MAX];
> > +};
> > +
> > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> > +		struct elm_errorvec *err_vec);
> > +struct device *elm_request(enum bch_ecc bch_type);
> > +#endif /* __ELM_H */
> > 
> 
> -- 
> 
> Peter Meerwald
> +43-664-2444418 (mobile)
> 

Re: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction

[Peter Korsgaard]

>>>>> Philip, Avinash <avinashphilip@ti.com> writes:

 > Platforms containing the ELM module can be used to correct errors
 > reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit
 > support is added.

This sounds odd to me. What about something like:

The ELM hardware module can be used to speedup BCH 4/8/16 ECC scheme
error correction.

For now only 4 & 8 bit support is added.


 > +++ b/drivers/mtd/devices/Makefile
 > @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
 >  obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
 >  obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
 >  obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
 > +obj-$(CONFIG_MTD_NAND_OMAP2)	+= elm.o

You seem to only use it in 4/4 if CONFIG_MTD_NAND_OMAP_BCH is set, so it
probably makes more sense to use that symbol to not needlessly include
it if it won't be used.


 > +++ b/drivers/mtd/devices/elm.c
 > @@ -0,0 +1,440 @@
 > +/*
 > + * Error Location Module
 > + *
 > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
 > + *
 > + * This program is free software; you can redistribute it and/or modify
 > + * it under the terms of the GNU General Public License as published by
 > + * the Free Software Foundation; either version 2 of the License, or
 > + * (at your option) any later version.
 > + *
 > + * This program is distributed in the hope that it will be useful,
 > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 > + * GNU General Public License for more details.
 > + *
 > + */
 > +
 > +#include <linux/platform_device.h>
 > +#include <linux/module.h>
 > +#include <linux/interrupt.h>
 > +#include <linux/io.h>
 > +#include <linux/of.h>
 > +#include <linux/pm_runtime.h>
 > +#include <linux/platform_data/elm.h>
 > +
 > +#define ELM_IRQSTATUS			0x018
 > +#define ELM_IRQENABLE			0x01c
 > +#define ELM_LOCATION_CONFIG		0x020
 > +#define ELM_PAGE_CTRL			0x080
 > +#define ELM_SYNDROME_FRAGMENT_0		0x400
 > +#define ELM_SYNDROME_FRAGMENT_6		0x418
 > +#define ELM_LOCATION_STATUS		0x800
 > +#define ELM_ERROR_LOCATION_0		0x880
 > +
 > +/* ELM Interrupt Status Register */
 > +#define INTR_STATUS_PAGE_VALID		BIT(8)
 > +
 > +/* ELM Interrupt Enable Register */
 > +#define INTR_EN_PAGE_MASK		BIT(8)
 > +
 > +/* ELM Location Configuration Register */
 > +#define ECC_BCH_LEVEL_MASK		0x3
 > +
 > +/* ELM syndrome */
 > +#define ELM_SYNDROME_VALID		BIT(16)
 > +
 > +/* ELM_LOCATION_STATUS Register */
 > +#define ECC_CORRECTABLE_MASK		BIT(8)
 > +#define ECC_NB_ERRORS_MASK		0x1f
 > +
 > +/* ELM_ERROR_LOCATION_0-15 Registers */
 > +#define ECC_ERROR_LOCATION_MASK		0x1fff
 > +
 > +#define ELM_ECC_SIZE			0x7ff
 > +
 > +#define SYNDROME_FRAGMENT_REG_SIZE	0x40
 > +#define ERROR_LOCATION_SIZE		0x100
 > +#define MAX_BCH_ELM_ERROR		16
 > +#define ELM_FRAGMENT_REG		7
 > +
 > +typedef u32 syn_t[ELM_FRAGMENT_REG];
 > +typedef u32 elm_error_t[MAX_BCH_ELM_ERROR];
 > +
 > +struct elm_info {
 > +	struct device *dev;
 > +	void __iomem *elm_base;
 > +	struct completion elm_completion;
 > +	struct list_head list;
 > +	enum bch_ecc bch_type;
 > +};
 > +
 > +static LIST_HEAD(elm_devices);
 > +
 > +static void elm_write_reg(void *offset, u32 val)
 > +{
 > +	writel(val, offset);
 > +}
 > +
 > +static u32 elm_read_reg(void *offset)
 > +{
 > +	return readl(offset);
 > +}

As written these read/write wrappers don't add anything. How about
passing struct elm_info and offset as an integer so you can drop
elm_base from all call sites, E.G.:

static void elm_write_reg(struct elm_info *info, int offset, u32 val)
{
        writel(val, info->elm_base + offset);
}


 > +
 > +/**
 > + * elm_config - Configure ELM module
 > + * @info:	elm info
 > + */
 > +static void elm_config(struct elm_info *info)
 > +{
 > +	u32 reg_val;
 > +
 > +	reg_val = (info->bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
 > +	elm_write_reg(info->elm_base + ELM_LOCATION_CONFIG, reg_val);
 > +}
 > +
 > +/**
 > + * elm_configure_page_mode - Enable/Disable page mode
 > + * @info:	elm info
 > + * @index:	index number of syndrome fragment vector
 > + * @enable:	enable/disable flag for page mode
 > + *
 > + * Enable page mode for syndrome fragment index
 > + */
 > +static void elm_configure_page_mode(struct elm_info *info, int index,
 > +		bool enable)
 > +{
 > +	u32 reg_val;
 > +
 > +	reg_val = elm_read_reg(info->elm_base + ELM_PAGE_CTRL);
 > +	if (enable)
 > +		reg_val |= BIT(index);	/* enable page mode */
 > +	else
 > +		reg_val &= ~BIT(index);	/* disable page mode */
 > +
 > +	elm_write_reg(info->elm_base + ELM_PAGE_CTRL, reg_val);
 > +}
 > +
 > +static void rearrange_bch4(u8 *syndrome)
 > +{
 > +	/*
 > +	 * BCH4 has 52 bit used for ecc, but OOB stored with
 > +	 * nibble 0 appended, removes appended 0 nibble
 > +	 */
 > +	u64 *dst = (u64 *)syndrome;
 > +	*dst >>= 4;
 > +}
 > +
 > +/**
 > + * elm_reverse_eccdata - Reverse ecc data
 > + * @info:	elm info
 > + * @ecc_data:	buffer for calculated ecc
 > + * @syndrome:	buffer for syndrome polynomial
 > + *
 > + * ecc_data has to be reversed for syndrome vector
 > + */
 > +static void elm_reverse_eccdata(struct elm_info *info, u8 *ecc_data,
 > +		u8 *syndrome)
 > +{
 > +	int i;
 > +	int bch_size = info->bch_type ? BCH8_ECC_OOB_BYTES : BCH4_SIZE;
 > +
 > +	for (i = 0; i < bch_size; i++)
 > +		syndrome[bch_size - 1 - i] = ecc_data[i];
 > +
 > +	/*
 > +	 * syndrome polynomial to be rearranged for BCH 4 ecc scheme as 52
 > +	 * bits being used and 4 bits being appended in syndrome vector
 > +	 */
 > +	if (info->bch_type == BCH4_ECC)
 > +		rearrange_bch4(syndrome);
 > +}
 > +
 > +/**
 > + * elm_load_syndrome - Load ELM syndrome reg
 > + * @info:	elm info
 > + * @index:	syndrome fragment index
 > + * @syndrome:	Syndrome polynomial
 > + *
 > + * Load syndrome fragment registers with syndrome polynomial
 > + */
 > +static void elm_load_syndrome(struct elm_info *info, int index, u8 *syndrome)
 > +{
 > +	int i;
 > +	int max = info->bch_type ? BCH8_SYNDROME_SIZE : BCH4_SYNDROME_SIZE;
 > +	void *syn_frag_base = info->elm_base + ELM_SYNDROME_FRAGMENT_0;
 > +	syn_t *syn_fragment;
 > +	u32 reg_val;
 > +
 > +	elm_configure_page_mode(info, index, true);
 > +	syn_fragment = syn_frag_base + SYNDROME_FRAGMENT_REG_SIZE * index;
 > +
 > +	for (i = 0; i < max; i++) {
 > +		reg_val = syndrome[0] | syndrome[1] << 8 | syndrome[2] << 16 |
 > +			syndrome[3] << 24;
 > +		elm_write_reg(*syn_fragment + i, reg_val);
 > +		/* Update syndrome polynomial pointer */
 > +		syndrome += 4;
 > +	}
 > +}
 > +
 > +/**
 > + * elm_start_processing - start elm syndrome processing
 > + * @info:	elm info
 > + * @err_vec:	elm error vectors
 > + *
 > + * Set syndrome valid bit for syndrome fragment registers for which
 > + * elm syndrome fragment registers are loaded. This enables elm module
 > + * to start processing syndrome vectors.
 > + */
 > +static void elm_start_processing(struct elm_info *info,
 > +		struct elm_errorvec *err_vec)
 > +{
 > +	int i;
 > +	void *offset;
 > +	u32 reg_val;
 > +
 > +	/*
 > +	 * Set syndrome vector valid so that ELM module will process it for
 > +	 * vectors error is reported
 > +	 */
 > +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
 > +		if (err_vec[i].error_reported) {
 > +			offset = info->elm_base + ELM_SYNDROME_FRAGMENT_6 + i *
 > +				SYNDROME_FRAGMENT_REG_SIZE;
 > +			reg_val = elm_read_reg(offset);
 > +			reg_val |= ELM_SYNDROME_VALID;
 > +			elm_write_reg(offset, reg_val);
 > +		}
 > +	}
 > +}
 > +
 > +/**
 > + * elm_error_correction - locate correctable error position
 > + * @info:	elm info
 > + * @err_vec:	elm error vectors
 > + *
 > + * On completion of processing by elm module, error location status
 > + * register updated with correctable/uncorrectable error information.
 > + * In case of correctable errors, number of errors located from
 > + * elm location status register & read the these many positions from
 > + * elm error location register.
 > + */
 > +static void elm_error_correction(struct elm_info *info,
 > +		struct elm_errorvec *err_vec)
 > +{
 > +	int i, j, errors = 0;
 > +	void *err_loc_base = info->elm_base + ELM_ERROR_LOCATION_0;
 > +	elm_error_t *err_loc;
 > +	void *offset;
 > +	u32 reg_val;
 > +
 > +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
 > +		/* check error reported */
 > +		if (err_vec[i].error_reported) {
 > +			offset = info->elm_base + ELM_LOCATION_STATUS +
 > +				i * ERROR_LOCATION_SIZE;
 > +			reg_val = elm_read_reg(offset);
 > +			/* check correctable error or not */
 > +			if (reg_val & ECC_CORRECTABLE_MASK) {
 > +				err_loc = err_loc_base +
 > +					i * ERROR_LOCATION_SIZE;
 > +				/* 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(*err_loc + j);
 > +					err_vec[i].error_loc[j] = reg_val &
 > +						ECC_ERROR_LOCATION_MASK;
 > +				}
 > +
 > +				errors += err_vec[i].error_count;
 > +			} else {
 > +				err_vec[i].error_uncorrectable++;
 > +			}
 > +
 > +			/* clearing interrupts for processed error vectors */
 > +			elm_write_reg(info->elm_base + ELM_IRQSTATUS, BIT(i));
 > +
 > +			/* disable page mode */
 > +			elm_configure_page_mode(info, i, false);
 > +		}
 > +	}
 > +
 > +	return;
 > +}
 > +
 > +/**
 > + * elm_decode_bch_error_page - Locate error position
 > + * @info:	elm info
 > + * @ecc_calc:	calculated ECC bytes from GPMC
 > + * @err_vec:	elm error vectors
 > + *
 > + * Called with one or greater reported and is vectors with error reported
 > + * is updated in err_vec[].error_reported
 > + */
 > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 > +		struct elm_errorvec *err_vec)
 > +{
 > +	int i;
 > +	u8 syndrome[BCH_MAX_ECC_BYTES_PER_SECTOR] = {0}, *syn_p;


Why do you need to keep the entire syndrome around? You seem to only use
it between elm_reverse_eccdata() and elm_load_syndrome(), so it could as
well be BCH8_ECC_OOB_BYTES long (or rather a multiple of sizeof(u32).

It would also be good to do the shuffeling directly in elm_load_syndrome
so you don't need the extra copy.


 > +	struct elm_info *info = dev_get_drvdata(dev);
 > +	u32 reg_val;
 > +
 > +	/* enable page mode interrupt */
 > +	reg_val = elm_read_reg(info->elm_base + ELM_IRQSTATUS);
 > +	elm_write_reg(info->elm_base + ELM_IRQSTATUS,
 > +			reg_val & INTR_STATUS_PAGE_VALID);
 > +	elm_write_reg(info->elm_base + ELM_IRQENABLE, INTR_EN_PAGE_MASK);
 > +
 > +	syn_p = syndrome;
 > +	for (i = 0; i < ERROR_VECTOR_MAX; i++) {
 > +		if (err_vec[i].error_reported) {
 > +			elm_reverse_eccdata(info, ecc_calc, syn_p);
 > +			elm_load_syndrome(info, i, syn_p);
 > +		}
 > +
 > +		ecc_calc += info->bch_type ? BCH8_SIZE : BCH4_SIZE;
 > +		syn_p += OOB_SECTOR_SIZE;
 > +	}
 > +
 > +	elm_start_processing(info, err_vec);
 > +
 > +	/*
 > +	 * In case of error reported, wait for ELM module to finish
 > +	 * locating error correction
 > +	 */
 > +	wait_for_completion(&info->elm_completion);
 > +
 > +	/* disable page mode interrupt */
 > +	reg_val = elm_read_reg(info->elm_base + ELM_IRQENABLE);
 > +	elm_write_reg(info->elm_base + ELM_IRQENABLE,
 > +			reg_val & ~INTR_EN_PAGE_MASK);
 > +	elm_error_correction(info, err_vec);
 > +}
 > +EXPORT_SYMBOL(elm_decode_bch_error_page);
 > +
 > +static irqreturn_t elm_isr(int this_irq, void *dev_id)
 > +{
 > +	u32 reg_val;
 > +	struct elm_info *info = dev_id;
 > +
 > +	reg_val = elm_read_reg(info->elm_base + ELM_IRQSTATUS);
 > +
 > +	/* all error vectors processed */
 > +	if (reg_val & INTR_STATUS_PAGE_VALID) {
 > +		elm_write_reg(info->elm_base + ELM_IRQSTATUS,
 > +				reg_val & INTR_STATUS_PAGE_VALID);
 > +		complete(&info->elm_completion);
 > +		return IRQ_HANDLED;
 > +	}
 > +
 > +	return IRQ_NONE;
 > +}
 > +
 > +struct device *elm_request(enum bch_ecc bch_type)
 > +{
 > +	struct elm_info *info;
 > +
 > +	list_for_each_entry(info, &elm_devices, list) {
 > +		if (info && info->dev) {
 > +				info->bch_type = bch_type;
 > +				elm_config(info);
 > +				return info->dev;
 > +		}
 > +	}
 > +
 > +	return NULL;
 > +}
 > +EXPORT_SYMBOL(elm_request);
 > +
 > +static int __devinit elm_probe(struct platform_device *pdev)
 > +{
 > +	int ret = 0;
 > +	struct resource *res, *irq;
 > +	struct elm_info *info;
 > +
 > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 > +	if (!info) {
 > +		dev_err(&pdev->dev, "failed to allocate memory\n");
 > +		return -ENOMEM;
 > +	}
 > +
 > +	info->dev = &pdev->dev;
 > +
 > +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 > +	if (!irq) {
 > +		dev_err(&pdev->dev, "no irq resource defined\n");
 > +		return -ENODEV;
 > +	}
 > +
 > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 > +	if (!res) {
 > +		dev_err(&pdev->dev, "no memory resource defined\n");
 > +		return -ENODEV;
 > +	}
 > +
 > +
 > +	info->elm_base = devm_request_and_ioremap(&pdev->dev, res);
 > +	if (!info->elm_base)
 > +		return -EADDRNOTAVAIL;
 > +
 > +
 > +	ret = devm_request_irq(&pdev->dev, irq->start, elm_isr, 0,
 > +			pdev->name, info);
 > +	if (ret) {
 > +		dev_err(&pdev->dev, "failure requesting irq %i\n", irq->start);
 > +		return ret;
 > +	}
 > +
 > +	pm_runtime_enable(&pdev->dev);
 > +	if (pm_runtime_get_sync(&pdev->dev)) {
 > +		ret = -EINVAL;
 > +		pm_runtime_disable(&pdev->dev);
 > +		dev_err(&pdev->dev, "can't enable clock\n");
 > +		return ret;
 > +	}
 > +
 > +	init_completion(&info->elm_completion);
 > +	INIT_LIST_HEAD(&info->list);
 > +	list_add(&info->list, &elm_devices);
 > +	platform_set_drvdata(pdev, info);
 > +	return ret;
 > +}
 > +
 > +static int __devexit elm_remove(struct platform_device *pdev)
 > +{
 > +	pm_runtime_put_sync(&pdev->dev);
 > +	pm_runtime_disable(&pdev->dev);
 > +	platform_set_drvdata(pdev, NULL);
 > +	return 0;
 > +}
 > +
 > +
 > +#ifdef CONFIG_OF
 > +static const struct of_device_id elm_of_match[] = {
 > +	{ .compatible = "ti,elm" },
 > +	{},
 > +};
 > +MODULE_DEVICE_TABLE(of, elm_of_match);
 > +#endif
 > +
 > +static struct platform_driver elm_driver = {
 > +	.driver		= {
 > +		.name	= "elm",
 > +		.of_match_table = of_match_ptr(elm_of_match),
 > +		.owner	= THIS_MODULE,
 > +	},
 > +	.probe		= elm_probe,
 > +	.remove		= __devexit_p(elm_remove),
 > +};
 > +
 > +module_platform_driver(elm_driver);
 > +
 > +MODULE_DESCRIPTION("ELM driver for BCH error correction");
 > +MODULE_AUTHOR("Texas Instruments");
 > +MODULE_ALIAS("platform: elm");
 > +MODULE_LICENSE("GPL v2");
 > diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
 > new file mode 100644
 > index 0000000..eb53163
 > --- /dev/null
 > +++ b/include/linux/platform_data/elm.h
 > @@ -0,0 +1,60 @@
 > +/*
 > + * BCH Error Location Module
 > + *
 > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
 > + *
 > + * This program is free software; you can redistribute it and/or modify
 > + * it under the terms of the GNU General Public License as published by
 > + * the Free Software Foundation; either version 2 of the License, or
 > + * (at your option) any later version.
 > + *
 > + * This program is distributed in the hope that it will be useful,
 > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 > + * GNU General Public License for more details.
 > + *
 > + */
 > +
 > +#ifndef __ELM_H
 > +#define __ELM_H
 > +
 > +enum bch_ecc {
 > +	BCH4_ECC = 0,
 > +	BCH8_ECC,
 > +	BCH16_ECC,

It probably makes more sense to not provide the enum value for BCH16 as
you don't support it.


 > +};
 > +
 > +/* ELM support 8 error syndrome process */
 > +#define ERROR_VECTOR_MAX		8
 > +#define OOB_SECTOR_SIZE			16
 > +
 > +#define BCH_MAX_ECC_BYTES_PER_SECTOR	(OOB_SECTOR_SIZE * ERROR_VECTOR_MAX)
 > +
 > +#define BCH8_ECC_OOB_BYTES		13
 > +/* RBL requires 14 byte even though BCH8 uses only 13 byte */
 > +#define BCH8_SIZE			(BCH8_ECC_OOB_BYTES + 1)
 > +#define BCH4_SIZE			7
 > +
 > +#define	BCH8_SYNDROME_SIZE		4	/* 13 bytes of ecc */
 > +#define	BCH4_SYNDROME_SIZE		2	/* 7 bytes of ecc */
 > +
 > +/**
 > + * struct elm_errorvec - error vector for elm
 > + * @error_reported:		set true for vectors error is reported
 > + *
 > + * @error_count:		number of correctable errors in the sector
 > + * @error_uncorrectable:	number of uncorrectable errors
 > + * @error_loc:			buffer for error location
 > + *
 > + */
 > +struct elm_errorvec {
 > +	bool error_reported;
 > +	int error_count;
 > +	int error_uncorrectable;
 > +	int error_loc[ERROR_VECTOR_MAX];
 > +};
 > +
 > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 > +		struct elm_errorvec *err_vec);
 > +struct device *elm_request(enum bch_ecc bch_type);
 > +#endif /* __ELM_H */
 > -- 
 > 1.7.0.4


 > ______________________________________________________
 > Linux MTD discussion mailing list
 > http://lists.infradead.org/mailman/listinfo/linux-mtd/


-- 
Bye, Peter Korsgaard

RE: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction

[Philip, Avinash]

On Tue, Oct 16, 2012 at 01:10:47, Peter Korsgaard wrote:
> >>>>> Philip, Avinash <avinashphilip@ti.com> writes:
> 
>  > Platforms containing the ELM module can be used to correct errors
>  > reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit
>  > support is added.
> 
> This sounds odd to me. What about something like:
> 
> The ELM hardware module can be used to speedup BCH 4/8/16 ECC scheme
> error correction.
> 
> For now only 4 & 8 bit support is added.

Ok I will correct it.

> 
> 
>  > +++ b/drivers/mtd/devices/Makefile
>  > @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART)		+= lart.o
>  >  obj-$(CONFIG_MTD_BLOCK2MTD)	+= block2mtd.o
>  >  obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
>  >  obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
>  > +obj-$(CONFIG_MTD_NAND_OMAP2)	+= elm.o
> 
> You seem to only use it in 4/4 if CONFIG_MTD_NAND_OMAP_BCH is set, so it
> probably makes more sense to use that symbol to not needlessly include
> it if it won't be used.

Ok. This been good.

> 
> 
>  > +static void elm_write_reg(void *offset, u32 val)
>  > +{
>  > +	writel(val, offset);
>  > +}
>  > +
>  > +static u32 elm_read_reg(void *offset)
>  > +{
>  > +	return readl(offset);
>  > +}
> 
> As written these read/write wrappers don't add anything. How about
> passing struct elm_info and offset as an integer so you can drop
> elm_base from all call sites, E.G.:
> 
> static void elm_write_reg(struct elm_info *info, int offset, u32 val)
> {
>         writel(val, info->elm_base + offset);
> }
>

Ok, this helps to reduce some indentation levels also.
 
> 
>  > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>  > +		struct elm_errorvec *err_vec)
>  > +{
>  > +	int i;
>  > +	u8 syndrome[BCH_MAX_ECC_BYTES_PER_SECTOR] = {0}, *syn_p;
> 
> 
> Why do you need to keep the entire syndrome around? You seem to only use
> it between elm_reverse_eccdata() and elm_load_syndrome(), so it could as
> well be BCH8_ECC_OOB_BYTES long (or rather a multiple of sizeof(u32).
> 
> It would also be good to do the shuffeling directly in elm_load_syndrome
> so you don't need the extra copy.

I will check.

> 
> 
>  > + */
>  > +
>  > +#ifndef __ELM_H
>  > +#define __ELM_H
>  > +
>  > +enum bch_ecc {
>  > +	BCH4_ECC = 0,
>  > +	BCH8_ECC,
>  > +	BCH16_ECC,
> 
> It probably makes more sense to not provide the enum value for BCH16 as
> you don't support it.
> 

Ok I will remove.

> 
>  > +};
>  > +

Thanks
Avinash

[PATCH 3/4] ARM: OMAP2: gpmc: Add support for BCH ECC scheme

[Philip, Avinash]

Add support for BCH ECC scheme to gpmc driver and also enabling multi
sector read/write. This helps in doing single shot NAND page read and
write.

ECC engine configurations
BCH 4 bit support
1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
13 and ecc_size1 as 1.

BCH 8 bit support
1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
26 and ecc_size1 as 2.

Note: For BCH8 ECC bytes set to 14 to make compatible with RBL.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 72428bd... c9bc3cf... M	arch/arm/mach-omap2/gpmc.c
:100644 100644 2e6e259... c916510... M	arch/arm/plat-omap/include/plat/gpmc.h
 arch/arm/mach-omap2/gpmc.c             |  120 +++++++++++++++++++++++++++++---
 arch/arm/plat-omap/include/plat/gpmc.h |    1 +
 2 files changed, 112 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 72428bd..c9bc3cf 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
+#include <linux/mtd/nand.h>
 
 #include <asm/mach-types.h>
 #include <plat/gpmc.h>
@@ -83,6 +84,18 @@
 #define ENABLE_PREFETCH		(0x1 << 7)
 #define DMA_MPU_MODE		2
 
+/* GPMC ecc engine settings for read */
+#define BCH_WRAPMODE_1		1	/* BCH wrap mode 6 */
+#define BCH8R_ECC_SIZE0		0x1a	/* ecc_size0 = 26 */
+#define BCH8R_ECC_SIZE1		0x2	/* ecc_size1 = 2 */
+#define BCH4R_ECC_SIZE0		0xd	/* ecc_size0 = 13 */
+#define BCH4R_ECC_SIZE1		0x1	/* ecc_size1 = 1 4bit padding in BCH4 */
+
+/* GPMC ecc engine settings for write */
+#define BCH_WRAPMODE_6		6	/* BCH wrap mode 6 */
+#define BCH_ECC_SIZE0		0x0	/* ecc_size0 = 0, no oob protection */
+#define BCH_ECC_SIZE1		0x20	/* ecc_size1 = 32 */
+
 /* XXX: Only NAND irq has been considered,currently these are the only ones used
  */
 #define	GPMC_NR_IRQ		2
@@ -1119,7 +1132,8 @@ EXPORT_SYMBOL_GPL(gpmc_init_hwecc_bch);
 int gpmc_enable_hwecc_bch(int cs, int mode, int dev_width, int nsectors,
 			  int nerrors)
 {
-	unsigned int val;
+	unsigned int val, wr_mode;
+	unsigned int ecc_size1, ecc_size0;
 
 	/* check if ecc module is in use */
 	if (gpmc_ecc_used != -EINVAL)
@@ -1130,18 +1144,35 @@ int gpmc_enable_hwecc_bch(int cs, int mode, int dev_width, int nsectors,
 	/* clear ecc and enable bits */
 	gpmc_write_reg(GPMC_ECC_CONTROL, 0x1);
 
-	/*
-	 * When using BCH, sector size is hardcoded to 512 bytes.
-	 * Here we are using wrapping mode 6 both for reading and writing, with:
-	 *  size0 = 0  (no additional protected byte in spare area)
-	 *  size1 = 32 (skip 32 nibbles = 16 bytes per sector in spare area)
-	 */
-	gpmc_write_reg(GPMC_ECC_SIZE_CONFIG, (32 << 22) | (0 << 12));
+	/*  When using BCH, sector size is hard coded to 512 bytes. */
+	if ((mode == NAND_ECC_READ) && (nsectors != 1)) {
+		/*
+		 * Here we are using wrapping mode 1 for reading, for
+		 * supporting multi sector reading.
+		 * Read mode, ECC engine enabled for valid ecc_size0 nibbles
+		 * & disabled for ecc_size1 nibbles.
+		 */
+		ecc_size0 = (nerrors == 8) ? BCH8R_ECC_SIZE0 : BCH4R_ECC_SIZE0;
+		ecc_size1 = (nerrors == 8) ? BCH8R_ECC_SIZE1 : BCH4R_ECC_SIZE1;
+		wr_mode = BCH_WRAPMODE_1;
+	} else {
+		/*
+		 * Here we are using wrapping mode 6 for writing,
+		 * ECC engine enabled for valid ecc_size0 nibbles
+		 * & disabled for ecc_size1 nibbles.
+		 */
+		ecc_size0 = BCH_ECC_SIZE0;
+		ecc_size1 = BCH_ECC_SIZE1;
+		wr_mode = BCH_WRAPMODE_6;
+	}
+
+	gpmc_write_reg(GPMC_ECC_SIZE_CONFIG, (ecc_size1 << 22) |
+			(ecc_size0 << 12));
 
 	/* BCH configuration */
 	val = ((1                        << 16) | /* enable BCH */
 	       (((nerrors == 8) ? 1 : 0) << 12) | /* 8 or 4 bits */
-	       (0x06                     <<  8) | /* wrap mode = 6 */
+	       (wr_mode                  <<  8) | /* wrap mode = 6 or 1 */
 	       (dev_width                <<  7) | /* bus width */
 	       (((nsectors-1) & 0x7)     <<  4) | /* number of sectors */
 	       (cs                       <<  1) | /* ECC CS */
@@ -1154,6 +1185,77 @@ int gpmc_enable_hwecc_bch(int cs, int mode, int dev_width, int nsectors,
 EXPORT_SYMBOL_GPL(gpmc_enable_hwecc_bch);
 
 /**
+ * gpmc_calculate_ecc_bch	- Generate ecc bytes per block of 512 data bytes for entire page
+ * @cs:  chip select number
+ * @dat: The pointer to data on which ECC is computed
+ * @ecc: The ECC output buffer
+ */
+int gpmc_calculate_ecc_bch(int cs, const u_char *dat, u_char *ecc)
+{
+	int i, eccbchtsel;
+	u32 nsectors, reg, bch_val1, bch_val2, bch_val3, bch_val4;
+
+	if (gpmc_ecc_used != cs)
+		return -EINVAL;
+
+	/* read number of sectors for ecc to be calculated */
+	nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
+	/*
+	 * find BCH scheme used
+	 * 0 -> BCH4
+	 * 1 -> BCH8
+	 */
+	eccbchtsel = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 12) & 0x3);
+
+	/* update ecc bytes for entire page */
+	for (i = 0; i < nsectors; i++) {
+
+		reg = GPMC_ECC_BCH_RESULT_0 + 16 * i;
+
+		/* Read hw-computed remainder */
+		bch_val1 = gpmc_read_reg(reg + 0);
+		bch_val2 = gpmc_read_reg(reg + 4);
+		if (eccbchtsel) {
+			bch_val3 = gpmc_read_reg(reg + 8);
+			bch_val4 = gpmc_read_reg(reg + 12);
+		}
+
+		if (eccbchtsel) {
+			/* BCH8 ecc scheme */
+			*ecc++ = (bch_val4 & 0xFF);
+			*ecc++ = ((bch_val3 >> 24) & 0xFF);
+			*ecc++ = ((bch_val3 >> 16) & 0xFF);
+			*ecc++ = ((bch_val3 >> 8) & 0xFF);
+			*ecc++ = (bch_val3 & 0xFF);
+			*ecc++ = ((bch_val2 >> 24) & 0xFF);
+			*ecc++ = ((bch_val2 >> 16) & 0xFF);
+			*ecc++ = ((bch_val2 >> 8) & 0xFF);
+			*ecc++ = (bch_val2 & 0xFF);
+			*ecc++ = ((bch_val1 >> 24) & 0xFF);
+			*ecc++ = ((bch_val1 >> 16) & 0xFF);
+			*ecc++ = ((bch_val1 >> 8) & 0xFF);
+			*ecc++ = (bch_val1 & 0xFF);
+			/* 14th byte of ecc not used */
+			*ecc++ = 0;
+		} else {
+			/* BCH4 ecc scheme */
+			*ecc++ = ((bch_val2 >> 12) & 0xFF);
+			*ecc++ = ((bch_val2 >> 4) & 0xFF);
+			*ecc++ = (((bch_val2 & 0xF) << 4) |
+					((bch_val1 >> 28) & 0xF));
+			*ecc++ = ((bch_val1 >> 20) & 0xFF);
+			*ecc++ = ((bch_val1 >> 12) & 0xFF);
+			*ecc++ = ((bch_val1 >> 4) & 0xFF);
+			*ecc++ = ((bch_val1 & 0xF) << 4);
+		}
+	}
+
+	gpmc_ecc_used = -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch);
+
+/**
  * gpmc_calculate_ecc_bch4 - Generate 7 ecc bytes per sector of 512 data bytes
  * @cs:  chip select number
  * @dat: The pointer to data on which ecc is computed
diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
index 2e6e259..c916510 100644
--- a/arch/arm/plat-omap/include/plat/gpmc.h
+++ b/arch/arm/plat-omap/include/plat/gpmc.h
@@ -185,6 +185,7 @@ int gpmc_enable_hwecc_bch(int cs, int mode, int dev_width, int nsectors,
 			  int nerrors);
 int gpmc_calculate_ecc_bch4(int cs, const u_char *dat, u_char *ecc);
 int gpmc_calculate_ecc_bch8(int cs, const u_char *dat, u_char *ecc);
+int gpmc_calculate_ecc_bch(int cs, const u_char *dat, u_char *ecc);
 #endif /* CONFIG_ARCH_OMAP3 */
 
 #endif
-- 
1.7.0.4

Re: [PATCH 3/4] ARM: OMAP2: gpmc: Add support for BCH ECC scheme

[Ivan Djelic]

On Wed, Oct 03, 2012 at 03:29:48PM +0100, Philip, Avinash wrote:
> Add support for BCH ECC scheme to gpmc driver and also enabling multi
> sector read/write. This helps in doing single shot NAND page read and
> write.
> 
> ECC engine configurations
> BCH 4 bit support
> 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
> 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
> 13 and ecc_size1 as 1.
> 
> BCH 8 bit support
> 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
> 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
> 26 and ecc_size1 as 2.
> 
> Note: For BCH8 ECC bytes set to 14 to make compatible with RBL.
> 

Hi Philip,

I have a few comments/questions below,

(...)
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 72428bd..c9bc3cf 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -24,6 +24,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> +#include <linux/mtd/nand.h>
>  
>  #include <asm/mach-types.h>
>  #include <plat/gpmc.h>
> @@ -83,6 +84,18 @@
>  #define ENABLE_PREFETCH		(0x1 << 7)
>  #define DMA_MPU_MODE		2
>  
> +/* GPMC ecc engine settings for read */
> +#define BCH_WRAPMODE_1		1	/* BCH wrap mode 6 */

Comment should say "mode 1".

(...)
>  /**
> + * gpmc_calculate_ecc_bch	- Generate ecc bytes per block of 512 data bytes for entire page
> + * @cs:  chip select number
> + * @dat: The pointer to data on which ECC is computed
> + * @ecc: The ECC output buffer
> + */
> +int gpmc_calculate_ecc_bch(int cs, const u_char *dat, u_char *ecc)
> +{
> +	int i, eccbchtsel;
> +	u32 nsectors, reg, bch_val1, bch_val2, bch_val3, bch_val4;
> +
> +	if (gpmc_ecc_used != cs)
> +		return -EINVAL;
> +
> +	/* read number of sectors for ecc to be calculated */
> +	nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
> +	/*
> +	 * find BCH scheme used
> +	 * 0 -> BCH4
> +	 * 1 -> BCH8
> +	 */
> +	eccbchtsel = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 12) & 0x3);
> +
> +	/* update ecc bytes for entire page */
> +	for (i = 0; i < nsectors; i++) {
> +
> +		reg = GPMC_ECC_BCH_RESULT_0 + 16 * i;
> +
> +		/* Read hw-computed remainder */
> +		bch_val1 = gpmc_read_reg(reg + 0);
> +		bch_val2 = gpmc_read_reg(reg + 4);
> +		if (eccbchtsel) {
> +			bch_val3 = gpmc_read_reg(reg + 8);
> +			bch_val4 = gpmc_read_reg(reg + 12);
> +		}
> +
> +		if (eccbchtsel) {
> +			/* BCH8 ecc scheme */
> +			*ecc++ = (bch_val4 & 0xFF);
> +			*ecc++ = ((bch_val3 >> 24) & 0xFF);
> +			*ecc++ = ((bch_val3 >> 16) & 0xFF);
> +			*ecc++ = ((bch_val3 >> 8) & 0xFF);
> +			*ecc++ = (bch_val3 & 0xFF);
> +			*ecc++ = ((bch_val2 >> 24) & 0xFF);
> +			*ecc++ = ((bch_val2 >> 16) & 0xFF);
> +			*ecc++ = ((bch_val2 >> 8) & 0xFF);
> +			*ecc++ = (bch_val2 & 0xFF);
> +			*ecc++ = ((bch_val1 >> 24) & 0xFF);
> +			*ecc++ = ((bch_val1 >> 16) & 0xFF);
> +			*ecc++ = ((bch_val1 >> 8) & 0xFF);
> +			*ecc++ = (bch_val1 & 0xFF);
> +			/* 14th byte of ecc not used */
> +			*ecc++ = 0;
> +		} else {
> +			/* BCH4 ecc scheme */
> +			*ecc++ = ((bch_val2 >> 12) & 0xFF);
> +			*ecc++ = ((bch_val2 >> 4) & 0xFF);
> +			*ecc++ = (((bch_val2 & 0xF) << 4) |
> +					((bch_val1 >> 28) & 0xF));
> +			*ecc++ = ((bch_val1 >> 20) & 0xFF);
> +			*ecc++ = ((bch_val1 >> 12) & 0xFF);
> +			*ecc++ = ((bch_val1 >> 4) & 0xFF);
> +			*ecc++ = ((bch_val1 & 0xF) << 4);
> +		}
> +	}
> +
> +	gpmc_ecc_used = -EINVAL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch);

Here you introduce a function very similar to gpmc_calculate_ecc_bch4 and
gpmc_calculate_ecc_bch8, but without the added benefit (IMHO) of the constant
polynomial that allows to get an ecc sequence of 0xFFs for a buffer filled with
0xFFs. Why ?
If using the ELM prevents you from reusing gpmc_calculate_ecc_bch[48], could you explain in which way ?

Best regards,
--
Ivan

RE: [PATCH 3/4] ARM: OMAP2: gpmc: Add support for BCH ECC scheme

[Philip, Avinash]

On Thu, Oct 04, 2012 at 00:24:58, Ivan Djelic wrote:
> On Wed, Oct 03, 2012 at 03:29:48PM +0100, Philip, Avinash wrote:
> > Add support for BCH ECC scheme to gpmc driver and also enabling multi
> > sector read/write. This helps in doing single shot NAND page read and
> > write.
> > 
> > ECC engine configurations
> > BCH 4 bit support
> > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
> > 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
> > 13 and ecc_size1 as 1.
> > 
> > BCH 8 bit support
> > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
> > 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
> > 26 and ecc_size1 as 2.
> > 
> > Note: For BCH8 ECC bytes set to 14 to make compatible with RBL.
> > 
> 
> Hi Philip,
> 
> I have a few comments/questions below,
> 
> (...)
> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> > index 72428bd..c9bc3cf 100644
> > --- a/arch/arm/mach-omap2/gpmc.c
> > +++ b/arch/arm/mach-omap2/gpmc.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/mtd/nand.h>
> >  
> >  #include <asm/mach-types.h>
> >  #include <plat/gpmc.h>
> > @@ -83,6 +84,18 @@
> >  #define ENABLE_PREFETCH		(0x1 << 7)
> >  #define DMA_MPU_MODE		2
> >  
> > +/* GPMC ecc engine settings for read */
> > +#define BCH_WRAPMODE_1		1	/* BCH wrap mode 6 */
> 
> Comment should say "mode 1".

Ok I will correct it.

> 
> (...)
> >  /**
> > + * gpmc_calculate_ecc_bch	- Generate ecc bytes per block of 512 data bytes for entire page
> > + * @cs:  chip select number
> > + * @dat: The pointer to data on which ECC is computed
> > + * @ecc: The ECC output buffer
> > + */
> > +int gpmc_calculate_ecc_bch(int cs, const u_char *dat, u_char *ecc)
> > +{
> > +	int i, eccbchtsel;
> > +	u32 nsectors, reg, bch_val1, bch_val2, bch_val3, bch_val4;
> > +
> > +	if (gpmc_ecc_used != cs)
> > +		return -EINVAL;
> > +
> > +	/* read number of sectors for ecc to be calculated */
> > +	nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
> > +	/*
> > +	 * find BCH scheme used
> > +	 * 0 -> BCH4
> > +	 * 1 -> BCH8
> > +	 */
> > +	eccbchtsel = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 12) & 0x3);
> > +
> > +	/* update ecc bytes for entire page */
> > +	for (i = 0; i < nsectors; i++) {
> > +
> > +		reg = GPMC_ECC_BCH_RESULT_0 + 16 * i;
> > +
> > +		/* Read hw-computed remainder */
> > +		bch_val1 = gpmc_read_reg(reg + 0);
> > +		bch_val2 = gpmc_read_reg(reg + 4);
> > +		if (eccbchtsel) {
> > +			bch_val3 = gpmc_read_reg(reg + 8);
> > +			bch_val4 = gpmc_read_reg(reg + 12);
> > +		}
> > +
> > +		if (eccbchtsel) {
> > +			/* BCH8 ecc scheme */
> > +			*ecc++ = (bch_val4 & 0xFF);
> > +			*ecc++ = ((bch_val3 >> 24) & 0xFF);
> > +			*ecc++ = ((bch_val3 >> 16) & 0xFF);
> > +			*ecc++ = ((bch_val3 >> 8) & 0xFF);
> > +			*ecc++ = (bch_val3 & 0xFF);
> > +			*ecc++ = ((bch_val2 >> 24) & 0xFF);
> > +			*ecc++ = ((bch_val2 >> 16) & 0xFF);
> > +			*ecc++ = ((bch_val2 >> 8) & 0xFF);
> > +			*ecc++ = (bch_val2 & 0xFF);
> > +			*ecc++ = ((bch_val1 >> 24) & 0xFF);
> > +			*ecc++ = ((bch_val1 >> 16) & 0xFF);
> > +			*ecc++ = ((bch_val1 >> 8) & 0xFF);
> > +			*ecc++ = (bch_val1 & 0xFF);
> > +			/* 14th byte of ecc not used */
> > +			*ecc++ = 0;
> > +		} else {
> > +			/* BCH4 ecc scheme */
> > +			*ecc++ = ((bch_val2 >> 12) & 0xFF);
> > +			*ecc++ = ((bch_val2 >> 4) & 0xFF);
> > +			*ecc++ = (((bch_val2 & 0xF) << 4) |
> > +					((bch_val1 >> 28) & 0xF));
> > +			*ecc++ = ((bch_val1 >> 20) & 0xFF);
> > +			*ecc++ = ((bch_val1 >> 12) & 0xFF);
> > +			*ecc++ = ((bch_val1 >> 4) & 0xFF);
> > +			*ecc++ = ((bch_val1 & 0xF) << 4);
> > +		}
> > +	}
> > +
> > +	gpmc_ecc_used = -EINVAL;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch);
> 
> Here you introduce a function very similar to gpmc_calculate_ecc_bch4 and
> gpmc_calculate_ecc_bch8, but without the added benefit (IMHO) of the constant
> polynomial that allows to get an ecc sequence of 0xFFs for a buffer filled with
> 0xFFs. Why ?

I don't exactly understand what we benefitted/achieve. In my observation,
this API does spare area also written with 0xFF if data area is 0xFFs.
So the area looks like erased page again.

> If using the ELM prevents you from reusing gpmc_calculate_ecc_bch[48], could you explain in which way ?

When using gpmc_calculate_ecc_bch[48], calculated ecc values modified.
The read sequence we following is
Read 512 byte -> read ECC bytes from spare area
Now the calculated ECC will be zero if no error is reported. In case of error, a syndrome
Polynomial is reported. In either case modifying will corrupt the data.

This is valid if we are writing a page with 0xFF also. But this time we were filling a valid
ecc in spare area not 0xFF as in gpmc_calculate_ecc_bch[48].

Additionally to make compatible with RBL ECC layout (14 byte), we were setting ecc[13] as zero.

Thanks
Avinash  

> 
> Best regards,
> --
> Ivan
> 

Re: [PATCH 3/4] ARM: OMAP2: gpmc: Add support for BCH ECC scheme

[Ivan Djelic]

On Thu, Oct 04, 2012 at 09:03:42AM +0100, Philip, Avinash wrote:
(...)
> > > +int gpmc_calculate_ecc_bch(int cs, const u_char *dat, u_char *ecc)
> > > +{
> > > +	int i, eccbchtsel;
> > > +	u32 nsectors, reg, bch_val1, bch_val2, bch_val3, bch_val4;
> > > +
> > > +	if (gpmc_ecc_used != cs)
> > > +		return -EINVAL;
> > > +
> > > +	/* read number of sectors for ecc to be calculated */
> > > +	nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
> > > +	/*
> > > +	 * find BCH scheme used
> > > +	 * 0 -> BCH4
> > > +	 * 1 -> BCH8
> > > +	 */
> > > +	eccbchtsel = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 12) & 0x3);
> > > +
> > > +	/* update ecc bytes for entire page */
> > > +	for (i = 0; i < nsectors; i++) {
> > > +
> > > +		reg = GPMC_ECC_BCH_RESULT_0 + 16 * i;
> > > +
> > > +		/* Read hw-computed remainder */
> > > +		bch_val1 = gpmc_read_reg(reg + 0);
> > > +		bch_val2 = gpmc_read_reg(reg + 4);
> > > +		if (eccbchtsel) {
> > > +			bch_val3 = gpmc_read_reg(reg + 8);
> > > +			bch_val4 = gpmc_read_reg(reg + 12);
> > > +		}
> > > +
> > > +		if (eccbchtsel) {
> > > +			/* BCH8 ecc scheme */
> > > +			*ecc++ = (bch_val4 & 0xFF);
> > > +			*ecc++ = ((bch_val3 >> 24) & 0xFF);
> > > +			*ecc++ = ((bch_val3 >> 16) & 0xFF);
> > > +			*ecc++ = ((bch_val3 >> 8) & 0xFF);
> > > +			*ecc++ = (bch_val3 & 0xFF);
> > > +			*ecc++ = ((bch_val2 >> 24) & 0xFF);
> > > +			*ecc++ = ((bch_val2 >> 16) & 0xFF);
> > > +			*ecc++ = ((bch_val2 >> 8) & 0xFF);
> > > +			*ecc++ = (bch_val2 & 0xFF);
> > > +			*ecc++ = ((bch_val1 >> 24) & 0xFF);
> > > +			*ecc++ = ((bch_val1 >> 16) & 0xFF);
> > > +			*ecc++ = ((bch_val1 >> 8) & 0xFF);
> > > +			*ecc++ = (bch_val1 & 0xFF);
> > > +			/* 14th byte of ecc not used */
> > > +			*ecc++ = 0;
> > > +		} else {
> > > +			/* BCH4 ecc scheme */
> > > +			*ecc++ = ((bch_val2 >> 12) & 0xFF);
> > > +			*ecc++ = ((bch_val2 >> 4) & 0xFF);
> > > +			*ecc++ = (((bch_val2 & 0xF) << 4) |
> > > +					((bch_val1 >> 28) & 0xF));
> > > +			*ecc++ = ((bch_val1 >> 20) & 0xFF);
> > > +			*ecc++ = ((bch_val1 >> 12) & 0xFF);
> > > +			*ecc++ = ((bch_val1 >> 4) & 0xFF);
> > > +			*ecc++ = ((bch_val1 & 0xF) << 4);
> > > +		}
> > > +	}
> > > +
> > > +	gpmc_ecc_used = -EINVAL;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch);
> > 
> > Here you introduce a function very similar to gpmc_calculate_ecc_bch4 and
> > gpmc_calculate_ecc_bch8, but without the added benefit (IMHO) of the constant
> > polynomial that allows to get an ecc sequence of 0xFFs for a buffer filled with
> > 0xFFs. Why ?
> 
> I don't exactly understand what we benefitted/achieve. In my observation,
> this API does spare area also written with 0xFF if data area is 0xFFs.
> So the area looks like erased page again.

Precisely. It means you can read the page with ECC enabled without having to check if
the page has been programmed; it also enables bitflip correction on erased pages.

> > If using the ELM prevents you from reusing gpmc_calculate_ecc_bch[48], could you explain in which way ?
> 
> When using gpmc_calculate_ecc_bch[48], calculated ecc values modified.
> The read sequence we following is
> Read 512 byte -> read ECC bytes from spare area
> Now the calculated ECC will be zero if no error is reported. In case of error, a syndrome
> Polynomial is reported. In either case modifying will corrupt the data.

It is still possible to retrieve your original error syndrome, even using the technique transforming ECC on erased pages into 0xFFs.
But I guess you're not interested if you need RBL compatibility.

BR,
--
Ivan

Re: [PATCH 3/4] ARM: OMAP2: gpmc: Add support for BCH ECC scheme

[Peter Korsgaard]

>>>>> Philip, Avinash <avinashphilip@ti.com> writes:

 > Add support for BCH ECC scheme to gpmc driver and also enabling multi
 > sector read/write. This helps in doing single shot NAND page read and
 > write.

 > ECC engine configurations
 > BCH 4 bit support
 > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
 > 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
 > 13 and ecc_size1 as 1.

 > BCH 8 bit support
 > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
 > 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
 > 26 and ecc_size1 as 2.

 > Note: For BCH8 ECC bytes set to 14 to make compatible with RBL.

On what device? In the am335x TRM (spruh73f.pdf) figure 26-15 (pg 4273)
the rom code is documented to not use any padding on the ECC bytes
(E.G. oob 2..53):

http://www.ti.com/litv/pdf/spruh73f

I see the driver in the u-boot-am33x tree (ti81xx_nand.c) seems to use
4x14 bytes as well though, so perhaps that's a bug in the documentation
instead?

-- 
Bye, Peter Korsgaard

RE: [PATCH 3/4] ARM: OMAP2: gpmc: Add support for BCH ECC scheme

[Philip, Avinash]

On Tue, Oct 16, 2012 at 00:18:30, Peter Korsgaard wrote:
> >>>>> Philip, Avinash <avinashphilip@ti.com> writes:
> 
>  > Add support for BCH ECC scheme to gpmc driver and also enabling multi
>  > sector read/write. This helps in doing single shot NAND page read and
>  > write.
> 
>  > ECC engine configurations
>  > BCH 4 bit support
>  > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
>  > 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
>  > 13 and ecc_size1 as 1.
> 
>  > BCH 8 bit support
>  > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
>  > 2. read  => ECC engine configured in wrap mode 1 and with ecc_size0 as
>  > 26 and ecc_size1 as 2.
> 
>  > Note: For BCH8 ECC bytes set to 14 to make compatible with RBL.
> 
> On what device? In the am335x TRM (spruh73f.pdf) figure 26-15 (pg 4273)
> the rom code is documented to not use any padding on the ECC bytes
> (E.G. oob 2..53):
> 
> http://www.ti.com/litv/pdf/spruh73f
> 
> I see the driver in the u-boot-am33x tree (ti81xx_nand.c) seems to use
> 4x14 bytes as well though, so perhaps that's a bug in the documentation
> instead?

Yes, RBL uses 4x14 bytes.

Thanks
Avinash

> 
> -- 
> Bye, Peter Korsgaard
> 

[PATCH 4/4] mtd: nand: omap2: Add data correction support

[Philip, Avinash]

ELM module can be used for error correction of BCH 4 & 8 bit. Also
support read & write page in one shot by adding custom read_page &
write_page methods. This helps in optimizing code.

New structure member "is_elm_used" is added to know the status of
whether the ELM module is used for error correction or not.

Note:
ECC layout of BCH8 uses 14 bytes for 512 byte of data to make compatible
with RBL ECC layout, even though the requirement was only 13 byte. This
results a common ecc layout across RBL, U-boot & Linux.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
:100644 100644 af511a9... 8fd6ddb... M	drivers/mtd/nand/omap2.c
:100644 100644 1a68c1e... 5b7054e... M	include/linux/platform_data/mtd-nand-omap2.h
 drivers/mtd/nand/omap2.c                     |  359 +++++++++++++++++++++++---
 include/linux/platform_data/mtd-nand-omap2.h |    1 +
 2 files changed, 328 insertions(+), 32 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index af511a9..8fd6ddb 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -30,6 +30,7 @@
 #include <plat/dma.h>
 #include <plat/gpmc.h>
 #include <linux/platform_data/mtd-nand-omap2.h>
+#include <linux/platform_data/elm.h>
 
 #define	DRIVER_NAME	"omap2-nand"
 #define	OMAP_NAND_TIMEOUT_MS	5000
@@ -114,6 +115,12 @@
 #define BCH8_MAX_ERROR		8	/* upto 8 bit coorectable */
 #define BCH4_MAX_ERROR		4	/* upto 4 bit correctable */
 
+#define SECTOR_BYTES		512
+/* 4 bit padding to make byte aligned, 56 = 52 + 4 */
+#define BCH4_BIT_PAD		4
+#define BCH8_ECC_MAX		((SECTOR_BYTES + BCH8_ECC_OOB_BYTES) * 8)
+#define BCH4_ECC_MAX		((SECTOR_BYTES + BCH4_SIZE) * 8)
+
 /* oob info generated runtime depending on ecc algorithm and layout selected */
 static struct nand_ecclayout omap_oobinfo;
 /* Define some generic bad / good block scan pattern which are used
@@ -153,6 +160,8 @@ struct omap_nand_info {
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
 	struct bch_control             *bch;
 	struct nand_ecclayout           ecclayout;
+	bool				is_elm_used;
+	struct device			*elm_dev;
 #endif
 };
 
@@ -892,6 +901,138 @@ static int omap_correct_data(struct mtd_info *mtd, u_char *dat,
 	return stat;
 }
 
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+/**
+ * omap_elm_correct_data - corrects page data area in case error reported
+ * @mtd:	MTD device structure
+ * @dat:	page data
+ * @read_ecc:	ecc read from nand flash
+ * @calc_ecc:	ecc read from HW ECC registers
+ *
+ * Check the read ecc vector from OOB area to see the page is flashed.
+ * If flashed, check any error reported by checking calculated ecc vector.
+ * For non error page, calculated ecc will be zero. For error pages,
+ * a non-zero valid syndrome polynomial reported in calculated ecc vector.
+ * Pass this non-zero syndrome polynomial to 'elm_decode_bch_error_page'
+ * with elm error vector updated for error reported sectors.
+ * On returning from this function, elm error vector updated with
+ * - number of correctable errors, error location if correctable.
+ * - if pages are non-correctable, updated with elm error vector
+ *   error uncorrectable.
+ */
+static int omap_elm_correct_data(struct mtd_info *mtd, u_char *dat,
+				u_char *read_ecc, u_char *calc_ecc)
+{
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+			mtd);
+	int eccsteps = info->nand.ecc.steps;
+	int i , j, stat = 0;
+	int eccsize, eccflag, size;
+	struct elm_errorvec err_vec[ERROR_VECTOR_MAX];
+	u_char *ecc_vec = calc_ecc;
+	enum bch_ecc type;
+	bool is_error_reported = false;
+
+	/* initialize elm error vector to zero */
+	memset(err_vec, 0, sizeof(err_vec));
+	if (info->nand.ecc.strength == BCH8_MAX_ERROR) {
+		size = BCH8_SIZE;
+		eccsize = BCH8_ECC_OOB_BYTES;
+		type = BCH8_ECC;
+	} else {
+		size = BCH4_SIZE;
+		eccsize = BCH4_SIZE;
+		type = BCH4_ECC;
+	}
+
+	for (i = 0; i < eccsteps ; i++) {
+		eccflag = 0;	/* initialize eccflag */
+
+		for (j = 0; (j < eccsize); j++) {
+			if (read_ecc[j] != 0xFF) {
+				eccflag = 1;	/* data area is flashed */
+				break;
+			}
+		}
+
+		/* check calculated ecc if data area is flashed */
+		if (eccflag == 1) {
+			eccflag = 0;
+			/*
+			 * check any error reported, in case of error
+			 * non zero ecc reported.
+			 */
+			for (j = 0; (j < eccsize); j++) {
+				if (calc_ecc[j] != 0) {
+					/* non zero ecc, error present */
+					eccflag = 1;
+					break;
+				}
+			}
+		}
+
+		/* update elm error vector */
+		if (eccflag == 1) {
+			err_vec[i].error_reported = true;
+			is_error_reported = true;
+		}
+
+		/* update the ecc vector */
+		calc_ecc = calc_ecc + size;
+		read_ecc = read_ecc + size;
+	}
+
+	/* Check if any error reported */
+	if (!is_error_reported)
+		return 0;
+
+	/* decode BCH error using ELM module */
+	elm_decode_bch_error_page(info->elm_dev, ecc_vec, err_vec);
+
+	for (i = 0; i < eccsteps; i++) {
+		if (err_vec[i].error_reported) {
+			for (j = 0; j < err_vec[i].error_count; j++) {
+				u32 bit_pos, byte_pos, error_max, pos;
+
+				if (type == BCH8_ECC)
+					error_max = BCH8_ECC_MAX;
+				else
+					error_max = BCH4_ECC_MAX;
+
+				if (info->nand.ecc.strength == BCH8_MAX_ERROR)
+					pos = err_vec[i].error_loc[j];
+				else
+					/* add 4 to take care 4 bit padding */
+					pos = err_vec[i].error_loc[j] +
+						BCH4_BIT_PAD;
+
+				/* calculate bit position of error */
+				bit_pos = pos % 8;
+				/* calculate byte position of error */
+				byte_pos = (error_max - pos - 1) / 8;
+
+				if (pos < error_max)
+					dat[byte_pos] ^= 1 << bit_pos;
+				/* else, not interested to correct ecc */
+			}
+
+			/* update number of correctable errors */
+			stat += err_vec[i].error_count;
+		}
+
+		/* update page data with sector size */
+		dat += info->nand.ecc.size;
+	}
+
+	for (i = 0; i < eccsteps; i++)
+		/* return error if uncorrectable error present */
+		if (err_vec[i].error_uncorrectable)
+			return -EINVAL;
+
+	return stat;
+}
+#endif
+
 /**
  * omap_calcuate_ecc - Generate non-inverted ECC bytes.
  * @mtd: MTD device structure
@@ -1039,14 +1180,45 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
 
 	nerrors = info->nand.ecc.strength;
 	dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+	if (info->is_elm_used) {
+		/*
+		 * Program GPMC to perform correction on (steps * 512) byte
+		 * sector at a time.
+		 */
+		gpmc_enable_hwecc_bch(info->gpmc_cs, mode, dev_width,
+				info->nand.ecc.steps, nerrors);
+		return;
+	}
+#endif
 	/*
-	 * Program GPMC to perform correction on one 512-byte sector at a time.
-	 * Using 4 sectors at a time (i.e. ecc.size = 2048) is also possible and
-	 * gives a slight (5%) performance gain (but requires additional code).
+	 * Program GPMC to perform correction on one 512-byte sector at
+	 * a time.
 	 */
-	(void)gpmc_enable_hwecc_bch(info->gpmc_cs, mode, dev_width, 1, nerrors);
+	(void)gpmc_enable_hwecc_bch(info->gpmc_cs, mode, dev_width, 1,
+				nerrors);
 }
 
+
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+/**
+ * omap3_calculate_ecc_bch - Generate bytes of ECC bytes
+ * @mtd:	MTD device structure
+ * @dat:	The pointer to data on which ecc is computed
+ * @ecc_code:	The ecc_code buffer
+ *
+ * support reading og BCH4/8 ecc vectors for the page
+ */
+static int omap3_calculate_ecc_bch(struct mtd_info *mtd, const u_char *dat,
+				    u_char *ecc_code)
+{
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+			mtd);
+
+	return gpmc_calculate_ecc_bch(info->gpmc_cs, dat, ecc_code);
+}
+#endif
+
 /**
  * omap3_calculate_ecc_bch4 - Generate 7 bytes of ECC bytes
  * @mtd: MTD device structure
@@ -1121,6 +1293,90 @@ static void omap3_free_bch(struct mtd_info *mtd)
 	}
 }
 
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+/**
+ * omap_write_page_bch - BCH ecc based write page function for entire page
+ * @mtd:		mtd info structure
+ * @chip:		nand chip info structure
+ * @buf:		data buffer
+ * @oob_required:	must write chip->oob_poi to OOB
+ *
+ * Custom write page method evolved to support multi sector writing in one shot
+ */
+static int omap_write_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
+				  const uint8_t *buf, int oob_required)
+{
+	int i;
+	uint8_t *ecc_calc = chip->buffers->ecccalc;
+	uint32_t *eccpos = chip->ecc.layout->eccpos;
+
+	/*
+	 * setting ecc vector with zero to support RBL compatibility.
+	 * RBL requires 14 byte of ecc with 14 byte as zero
+	 * even though BCH8 requires only 13 byte of ecc bytes.
+	 */
+	memset(ecc_calc, 0x0, chip->ecc.total);
+	chip->ecc.hwctl(mtd, NAND_ECC_WRITE);
+	chip->write_buf(mtd, buf, mtd->writesize);
+	chip->ecc.calculate(mtd, buf, &ecc_calc[0]);
+
+	for (i = 0; i < chip->ecc.total; i++)
+		chip->oob_poi[eccpos[i]] = ecc_calc[i];
+
+	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+	return 0;
+}
+
+/**
+ * omap_read_page_bch - BCH ecc based page read function for entire page
+ * @mtd:		mtd info structure
+ * @chip:		nand chip info structure
+ * @buf:		buffer to store read data
+ * @oob_required:	caller requires OOB data read to chip->oob_poi
+ * @page:		page number to read
+ *
+ * For BCH ecc scheme, GPMC used for syndrome calculation and ELM module
+ * used for error correction.
+ * Custom method evolved to support ELM error correction. On reading page
+ * data area is read along with OOB data with ecc engine enabled. ecc vector
+ * updated after read of OOB data. For non error pages ecc vector reported as
+ * zero.
+ */
+static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
+				uint8_t *buf, int oob_required, int page)
+{
+	uint8_t *ecc_calc = chip->buffers->ecccalc;
+	uint8_t *ecc_code = chip->buffers->ecccode;
+	uint32_t *eccpos = chip->ecc.layout->eccpos;
+	uint8_t *oob = &chip->oob_poi[eccpos[0]];
+	uint32_t oob_pos = mtd->writesize + chip->ecc.layout->eccpos[0];
+	int stat;
+
+	/* enable GPMC ecc engine */
+	chip->ecc.hwctl(mtd, NAND_ECC_READ);
+	/* read data */
+	chip->read_buf(mtd, buf, mtd->writesize);
+
+	/* read oob bytes */
+	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, oob_pos, -1);
+	chip->read_buf(mtd, oob, chip->ecc.total);
+
+	/* calculate ecc bytes */
+	chip->ecc.calculate(mtd, buf, ecc_calc);
+
+	memcpy(ecc_code, &chip->oob_poi[eccpos[0]], chip->ecc.total);
+
+	stat = chip->ecc.correct(mtd, buf, ecc_code, ecc_calc);
+
+	if (stat < 0)
+		mtd->ecc_stats.failed++;
+	else
+		mtd->ecc_stats.corrected += stat;
+
+	return 0;
+}
+#endif
+
 /**
  * omap3_init_bch - Initialize BCH ECC
  * @mtd: MTD device structure
@@ -1146,35 +1402,62 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
 		goto fail;
 	}
 
-	/* initialize GPMC BCH engine */
-	ret = gpmc_init_hwecc_bch(info->gpmc_cs, 1, max_errors);
-	if (ret)
-		goto fail;
-
-	/* software bch library is only used to detect and locate errors */
-	info->bch = init_bch(13, max_errors, 0x201b /* hw polynomial */);
-	if (!info->bch)
-		goto fail;
+	info->nand.ecc.size = 512;
+	info->nand.ecc.hwctl = omap3_enable_hwecc_bch;
+	info->nand.ecc.mode = NAND_ECC_HW;
+	info->nand.ecc.strength = hw_errors;
 
-	info->nand.ecc.size    = 512;
-	info->nand.ecc.hwctl   = omap3_enable_hwecc_bch;
-	info->nand.ecc.correct = omap3_correct_data_bch;
-	info->nand.ecc.mode    = NAND_ECC_HW;
+	if (info->is_elm_used && (mtd->writesize <= 4096)) {
+		enum bch_ecc bch_type;
 
-	/*
-	 * The number of corrected errors in an ecc block that will trigger
-	 * block scrubbing defaults to the ecc strength (4 or 8).
-	 * Set mtd->bitflip_threshold here to define a custom threshold.
-	 */
+		if (hw_errors == BCH8_MAX_ERROR) {
+			bch_type = BCH8_ECC;
+			info->nand.ecc.bytes = BCH8_SIZE;
+		} else {
+			bch_type = BCH4_ECC;
+			info->nand.ecc.bytes = BCH4_SIZE;
+		}
 
-	if (max_errors == 8) {
-		info->nand.ecc.strength  = 8;
-		info->nand.ecc.bytes     = 13;
-		info->nand.ecc.calculate = omap3_calculate_ecc_bch8;
+		info->nand.ecc.correct = omap_elm_correct_data;
+		info->nand.ecc.calculate = omap3_calculate_ecc_bch;
+		info->nand.ecc.read_page = omap_read_page_bch;
+		info->nand.ecc.write_page = omap_write_page_bch;
+		info->elm_dev = elm_request(bch_type);
+		if (!info->elm_dev) {
+			pr_err("Request to elm module failed\n");
+			goto fail;
+		}
 	} else {
-		info->nand.ecc.strength  = 4;
-		info->nand.ecc.bytes     = 7;
-		info->nand.ecc.calculate = omap3_calculate_ecc_bch4;
+
+		/* initialize GPMC BCH engine */
+		ret = gpmc_init_hwecc_bch(info->gpmc_cs, 1, max_errors);
+		if (ret)
+			goto fail;
+
+		/*
+		 * software bch library is only used to detect and
+		 * locateerrors
+		 */
+		info->bch = init_bch(13, max_errors,
+				0x201b /* hw polynomial */);
+		if (!info->bch)
+			goto fail;
+
+		info->nand.ecc.correct = omap3_correct_data_bch;
+
+		/*
+		 * The number of corrected errors in an ecc block that will
+		 * trigger block scrubbing defaults to the ecc strength (4 or 8)
+		 * Set mtd->bitflip_threshold here to define a custom threshold.
+		 */
+
+		if (max_errors == 8) {
+			info->nand.ecc.bytes = 13;
+			info->nand.ecc.calculate = omap3_calculate_ecc_bch8;
+		} else {
+			info->nand.ecc.bytes = 7;
+			info->nand.ecc.calculate = omap3_calculate_ecc_bch4;
+		}
 	}
 
 	pr_info("enabling NAND BCH ecc with %d-bit correction\n", max_errors);
@@ -1190,7 +1473,7 @@ fail:
  */
 static int omap3_init_bch_tail(struct mtd_info *mtd)
 {
-	int i, steps;
+	int i, steps, offset;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 						   mtd);
 	struct nand_ecclayout *layout = &info->ecclayout;
@@ -1212,11 +1495,20 @@ static int omap3_init_bch_tail(struct mtd_info *mtd)
 		goto fail;
 	}
 
+	/* ECC layout compatible with RBL for BCH8 */
+	if (info->is_elm_used && (info->nand.ecc.bytes == BCH8_SIZE))
+		offset = 2;
+	else
+		offset = mtd->oobsize - layout->eccbytes;
 	/* put ecc bytes at oob tail */
 	for (i = 0; i < layout->eccbytes; i++)
-		layout->eccpos[i] = mtd->oobsize-layout->eccbytes+i;
+		layout->eccpos[i] = offset + i;
+
+	if (info->is_elm_used && (info->nand.ecc.bytes == BCH8_SIZE))
+		layout->oobfree[0].offset = 2 + layout->eccbytes * steps;
+	else
+		layout->oobfree[0].offset = 2;
 
-	layout->oobfree[0].offset = 2;
 	layout->oobfree[0].length = mtd->oobsize-2-layout->eccbytes;
 	info->nand.ecc.layout = layout;
 
@@ -1279,6 +1571,9 @@ static int __devinit omap_nand_probe(struct platform_device *pdev)
 
 	info->nand.options	= pdata->devsize;
 	info->nand.options	|= NAND_SKIP_BBTSCAN;
+#ifdef CONFIG_MTD_NAND_OMAP_BCH
+	info->is_elm_used	= pdata->is_elm_used;
+#endif
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res == NULL) {
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 1a68c1e..5b7054e 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -28,6 +28,7 @@ struct omap_nand_platform_data {
 	int			devsize;
 	enum omap_ecc           ecc_opt;
 	struct gpmc_nand_regs	reg;
+	bool			is_elm_used;
 };
 
 /* minimum size for IO mapping */
-- 
1.7.0.4

Re: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Ivan Djelic]

On Wed, Oct 03, 2012 at 03:29:49PM +0100, Philip, Avinash wrote:
> ELM module can be used for error correction of BCH 4 & 8 bit. Also
> support read & write page in one shot by adding custom read_page &
> write_page methods. This helps in optimizing code.
> 
> New structure member "is_elm_used" is added to know the status of
> whether the ELM module is used for error correction or not.
> 
> Note:
> ECC layout of BCH8 uses 14 bytes for 512 byte of data to make compatible
> with RBL ECC layout, even though the requirement was only 13 byte. This
> results a common ecc layout across RBL, U-boot & Linux.
> 

See a few comments below,

(...)
> +static int omap_elm_correct_data(struct mtd_info *mtd, u_char *dat,
> +                               u_char *read_ecc, u_char *calc_ecc)
> +{
> +       struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> +                       mtd);
> +       int eccsteps = info->nand.ecc.steps;
> +       int i , j, stat = 0;
> +       int eccsize, eccflag, size;
> +       struct elm_errorvec err_vec[ERROR_VECTOR_MAX];
> +       u_char *ecc_vec = calc_ecc;
> +       enum bch_ecc type;
> +       bool is_error_reported = false;
> +
> +       /* initialize elm error vector to zero */
> +       memset(err_vec, 0, sizeof(err_vec));
> +       if (info->nand.ecc.strength == BCH8_MAX_ERROR) {
> +               size = BCH8_SIZE;
> +               eccsize = BCH8_ECC_OOB_BYTES;
> +               type = BCH8_ECC;
> +       } else {
> +               size = BCH4_SIZE;
> +               eccsize = BCH4_SIZE;
> +               type = BCH4_ECC;
> +       }
> +
> +       for (i = 0; i < eccsteps ; i++) {
> +               eccflag = 0;    /* initialize eccflag */
> +
> +               for (j = 0; (j < eccsize); j++) {
> +                       if (read_ecc[j] != 0xFF) {
> +                               eccflag = 1;    /* data area is flashed */

Just a reminder: this way of checking if a page has been programmed is not robust to bitflips,
so you may get into trouble with UBIFS on a fairly recent device.

(...)
> @@ -1039,14 +1180,45 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
> 
>         nerrors = info->nand.ecc.strength;
>         dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
> +#ifdef CONFIG_MTD_NAND_OMAP_BCH
> +       if (info->is_elm_used) {
> +               /*
> +                * Program GPMC to perform correction on (steps * 512) byte
> +                * sector at a time.
> +                */
> +               gpmc_enable_hwecc_bch(info->gpmc_cs, mode, dev_width,
> +                               info->nand.ecc.steps, nerrors);
> +               return;
> +       }
> +#endif
>         /*
> -        * Program GPMC to perform correction on one 512-byte sector at a time.
> -        * Using 4 sectors at a time (i.e. ecc.size = 2048) is also possible and
> -        * gives a slight (5%) performance gain (but requires additional code).
> +        * Program GPMC to perform correction on one 512-byte sector at
> +        * a time.

Why removing the comment about 4-sector perf gain ? :-)

(...)
> @@ -1146,35 +1402,62 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
>                 goto fail;
>         }
> 
> -       /* initialize GPMC BCH engine */
> -       ret = gpmc_init_hwecc_bch(info->gpmc_cs, 1, max_errors);
> -       if (ret)
> -               goto fail;
> -
> -       /* software bch library is only used to detect and locate errors */
> -       info->bch = init_bch(13, max_errors, 0x201b /* hw polynomial */);
> -       if (!info->bch)
> -               goto fail;
> +       info->nand.ecc.size = 512;
> +       info->nand.ecc.hwctl = omap3_enable_hwecc_bch;
> +       info->nand.ecc.mode = NAND_ECC_HW;
> +       info->nand.ecc.strength = hw_errors;
> 
> -       info->nand.ecc.size    = 512;
> -       info->nand.ecc.hwctl   = omap3_enable_hwecc_bch;
> -       info->nand.ecc.correct = omap3_correct_data_bch;
> -       info->nand.ecc.mode    = NAND_ECC_HW;
> +       if (info->is_elm_used && (mtd->writesize <= 4096)) {
> +               enum bch_ecc bch_type;
> 
> -       /*
> -        * The number of corrected errors in an ecc block that will trigger
> -        * block scrubbing defaults to the ecc strength (4 or 8).
> -        * Set mtd->bitflip_threshold here to define a custom threshold.
> -        */
> +               if (hw_errors == BCH8_MAX_ERROR) {
> +                       bch_type = BCH8_ECC;
> +                       info->nand.ecc.bytes = BCH8_SIZE;
> +               } else {
> +                       bch_type = BCH4_ECC;
> +                       info->nand.ecc.bytes = BCH4_SIZE;
> +               }
> 
> -       if (max_errors == 8) {
> -               info->nand.ecc.strength  = 8;
> -               info->nand.ecc.bytes     = 13;
> -               info->nand.ecc.calculate = omap3_calculate_ecc_bch8;
> +               info->nand.ecc.correct = omap_elm_correct_data;
> +               info->nand.ecc.calculate = omap3_calculate_ecc_bch;
> +               info->nand.ecc.read_page = omap_read_page_bch;
> +               info->nand.ecc.write_page = omap_write_page_bch;
> +               info->elm_dev = elm_request(bch_type);
> +               if (!info->elm_dev) {
> +                       pr_err("Request to elm module failed\n");
> +                       goto fail;
> +               }
>         } else {
> -               info->nand.ecc.strength  = 4;
> -               info->nand.ecc.bytes     = 7;
> -               info->nand.ecc.calculate = omap3_calculate_ecc_bch4;
> +
> +               /* initialize GPMC BCH engine */
> +               ret = gpmc_init_hwecc_bch(info->gpmc_cs, 1, max_errors);
> +               if (ret)
> +                       goto fail;
> +
> +               /*
> +                * software bch library is only used to detect and
> +                * locateerrors

s/locateerrors/locate errors/

BTW, did you check that your patch does not break the software BCH case (i.e. no ELM) ?

BR,
--
Ivan

RE: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Philip, Avinash]

On Thu, Oct 04, 2012 at 00:50:45, Ivan Djelic wrote:
> On Wed, Oct 03, 2012 at 03:29:49PM +0100, Philip, Avinash wrote:
> > ELM module can be used for error correction of BCH 4 & 8 bit. Also
> > support read & write page in one shot by adding custom read_page &
> > write_page methods. This helps in optimizing code.
> > 
> > New structure member "is_elm_used" is added to know the status of
> > whether the ELM module is used for error correction or not.
> > 
> > Note:
> > ECC layout of BCH8 uses 14 bytes for 512 byte of data to make compatible
> > with RBL ECC layout, even though the requirement was only 13 byte. This
> > results a common ecc layout across RBL, U-boot & Linux.
> > 
> 
> See a few comments below,
> 
> (...)
> > +static int omap_elm_correct_data(struct mtd_info *mtd, u_char *dat,
> > +                               u_char *read_ecc, u_char *calc_ecc)
> > +{
> > +       struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> > +                       mtd);
> > +       int eccsteps = info->nand.ecc.steps;
> > +       int i , j, stat = 0;
> > +       int eccsize, eccflag, size;
> > +       struct elm_errorvec err_vec[ERROR_VECTOR_MAX];
> > +       u_char *ecc_vec = calc_ecc;
> > +       enum bch_ecc type;
> > +       bool is_error_reported = false;
> > +
> > +       /* initialize elm error vector to zero */
> > +       memset(err_vec, 0, sizeof(err_vec));
> > +       if (info->nand.ecc.strength == BCH8_MAX_ERROR) {
> > +               size = BCH8_SIZE;
> > +               eccsize = BCH8_ECC_OOB_BYTES;
> > +               type = BCH8_ECC;
> > +       } else {
> > +               size = BCH4_SIZE;
> > +               eccsize = BCH4_SIZE;
> > +               type = BCH4_ECC;
> > +       }
> > +
> > +       for (i = 0; i < eccsteps ; i++) {
> > +               eccflag = 0;    /* initialize eccflag */
> > +
> > +               for (j = 0; (j < eccsize); j++) {
> > +                       if (read_ecc[j] != 0xFF) {
> > +                               eccflag = 1;    /* data area is flashed */
> 
> Just a reminder: this way of checking if a page has been programmed is not robust to bitflips,
> so you may get into trouble with UBIFS on a fairly recent device.
> 
> (...)
> > @@ -1039,14 +1180,45 @@ static void omap3_enable_hwecc_bch(struct mtd_info *mtd, int mode)
> > 
> >         nerrors = info->nand.ecc.strength;
> >         dev_width = (chip->options & NAND_BUSWIDTH_16) ? 1 : 0;
> > +#ifdef CONFIG_MTD_NAND_OMAP_BCH
> > +       if (info->is_elm_used) {
> > +               /*
> > +                * Program GPMC to perform correction on (steps * 512) byte
> > +                * sector at a time.
> > +                */
> > +               gpmc_enable_hwecc_bch(info->gpmc_cs, mode, dev_width,
> > +                               info->nand.ecc.steps, nerrors);
> > +               return;
> > +       }
> > +#endif
> >         /*
> > -        * Program GPMC to perform correction on one 512-byte sector at a time.
> > -        * Using 4 sectors at a time (i.e. ecc.size = 2048) is also possible and
> > -        * gives a slight (5%) performance gain (but requires additional code).
> > +        * Program GPMC to perform correction on one 512-byte sector at
> > +        * a time.
> 
> Why removing the comment about 4-sector perf gain ? :-)

With this patch, support for reading 4 sectors (max 8) is available.
Hence I am removing it.

> 
> (...)
> > @@ -1146,35 +1402,62 @@ static int omap3_init_bch(struct mtd_info *mtd, int ecc_opt)
> >                 goto fail;
> >         }
> > 
> > -       /* initialize GPMC BCH engine */
> > -       ret = gpmc_init_hwecc_bch(info->gpmc_cs, 1, max_errors);
> > -       if (ret)
> > -               goto fail;
> > -
> > -       /* software bch library is only used to detect and locate errors */
> > -       info->bch = init_bch(13, max_errors, 0x201b /* hw polynomial */);
> > -       if (!info->bch)
> > -               goto fail;
> > +       info->nand.ecc.size = 512;
> > +       info->nand.ecc.hwctl = omap3_enable_hwecc_bch;
> > +       info->nand.ecc.mode = NAND_ECC_HW;
> > +       info->nand.ecc.strength = hw_errors;
> > 
> > -       info->nand.ecc.size    = 512;
> > -       info->nand.ecc.hwctl   = omap3_enable_hwecc_bch;
> > -       info->nand.ecc.correct = omap3_correct_data_bch;
> > -       info->nand.ecc.mode    = NAND_ECC_HW;
> > +       if (info->is_elm_used && (mtd->writesize <= 4096)) {
> > +               enum bch_ecc bch_type;
> > 
> > -       /*
> > -        * The number of corrected errors in an ecc block that will trigger
> > -        * block scrubbing defaults to the ecc strength (4 or 8).
> > -        * Set mtd->bitflip_threshold here to define a custom threshold.
> > -        */
> > +               if (hw_errors == BCH8_MAX_ERROR) {
> > +                       bch_type = BCH8_ECC;
> > +                       info->nand.ecc.bytes = BCH8_SIZE;
> > +               } else {
> > +                       bch_type = BCH4_ECC;
> > +                       info->nand.ecc.bytes = BCH4_SIZE;
> > +               }
> > 
> > -       if (max_errors == 8) {
> > -               info->nand.ecc.strength  = 8;
> > -               info->nand.ecc.bytes     = 13;
> > -               info->nand.ecc.calculate = omap3_calculate_ecc_bch8;
> > +               info->nand.ecc.correct = omap_elm_correct_data;
> > +               info->nand.ecc.calculate = omap3_calculate_ecc_bch;
> > +               info->nand.ecc.read_page = omap_read_page_bch;
> > +               info->nand.ecc.write_page = omap_write_page_bch;
> > +               info->elm_dev = elm_request(bch_type);
> > +               if (!info->elm_dev) {
> > +                       pr_err("Request to elm module failed\n");
> > +                       goto fail;
> > +               }
> >         } else {
> > -               info->nand.ecc.strength  = 4;
> > -               info->nand.ecc.bytes     = 7;
> > -               info->nand.ecc.calculate = omap3_calculate_ecc_bch4;
> > +
> > +               /* initialize GPMC BCH engine */
> > +               ret = gpmc_init_hwecc_bch(info->gpmc_cs, 1, max_errors);
> > +               if (ret)
> > +                       goto fail;
> > +
> > +               /*
> > +                * software bch library is only used to detect and
> > +                * locateerrors
> 
> s/locateerrors/locate errors/

Ok I will correct it.

> 
> BTW, did you check that your patch does not break the software BCH case (i.e. no ELM) ?

I had checked in AM335x-evm software BCH without ELM. So this patch set is not breaking
software BCH functionality.

Thanks
Avinash


> 
> BR,
> --
> Ivan
> 

RE: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Philip, Avinash]

On Thu, Oct 04, 2012 at 15:51:03, Philip, Avinash wrote:
> On Thu, Oct 04, 2012 at 00:50:45, Ivan Djelic wrote:
> > On Wed, Oct 03, 2012 at 03:29:49PM +0100, Philip, Avinash wrote:
> > > ELM module can be used for error correction of BCH 4 & 8 bit. Also
> > > support read & write page in one shot by adding custom read_page &
> > > write_page methods. This helps in optimizing code.
> > > 
> > > New structure member "is_elm_used" is added to know the status of
> > > whether the ELM module is used for error correction or not.
> > > 
> > > Note:
> > > ECC layout of BCH8 uses 14 bytes for 512 byte of data to make compatible
> > > with RBL ECC layout, even though the requirement was only 13 byte. This
> > > results a common ecc layout across RBL, U-boot & Linux.
> > > 
> > 
> > See a few comments below,
> > 
> > (...)
> > > +static int omap_elm_correct_data(struct mtd_info *mtd, u_char *dat,
> > > +                               u_char *read_ecc, u_char *calc_ecc)
> > > +{
> > > +       struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> > > +                       mtd);
> > > +       int eccsteps = info->nand.ecc.steps;
> > > +       int i , j, stat = 0;
> > > +       int eccsize, eccflag, size;
> > > +       struct elm_errorvec err_vec[ERROR_VECTOR_MAX];
> > > +       u_char *ecc_vec = calc_ecc;
> > > +       enum bch_ecc type;
> > > +       bool is_error_reported = false;
> > > +
> > > +       /* initialize elm error vector to zero */
> > > +       memset(err_vec, 0, sizeof(err_vec));
> > > +       if (info->nand.ecc.strength == BCH8_MAX_ERROR) {
> > > +               size = BCH8_SIZE;
> > > +               eccsize = BCH8_ECC_OOB_BYTES;
> > > +               type = BCH8_ECC;
> > > +       } else {
> > > +               size = BCH4_SIZE;
> > > +               eccsize = BCH4_SIZE;
> > > +               type = BCH4_ECC;
> > > +       }
> > > +
> > > +       for (i = 0; i < eccsteps ; i++) {
> > > +               eccflag = 0;    /* initialize eccflag */
> > > +
> > > +               for (j = 0; (j < eccsize); j++) {
> > > +                       if (read_ecc[j] != 0xFF) {
> > > +                               eccflag = 1;    /* data area is flashed */
> > 
> > Just a reminder: this way of checking if a page has been programmed is not robust to bitflips,
> > so you may get into trouble with UBIFS on a fairly recent device.

Sorry I missed this point.

Here we were checking data in spare area (only in ecc layout 14*4 for BCH8) is 0xFF. If all data
in spare area is 0xFF, conclude that the page is erased & no need of error correction. In case
of bit flip present in spare area, page will be reported as uncorrectable.
But I am not understand understand " trouble with UBIFS on a fairly recent device".
Can you little elaborativ

Thanks
Avinash 

Re: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Ivan Djelic]

On Fri, Oct 05, 2012 at 09:51:50AM +0100, Philip, Avinash wrote:
> On Thu, Oct 04, 2012 at 15:51:03, Philip, Avinash wrote:
> > On Thu, Oct 04, 2012 at 00:50:45, Ivan Djelic wrote:
> > > On Wed, Oct 03, 2012 at 03:29:49PM +0100, Philip, Avinash wrote:
> > > > ELM module can be used for error correction of BCH 4 & 8 bit. Also
> > > > support read & write page in one shot by adding custom read_page &
> > > > write_page methods. This helps in optimizing code.
> > > > 
> > > > New structure member "is_elm_used" is added to know the status of
> > > > whether the ELM module is used for error correction or not.
> > > > 
> > > > Note:
> > > > ECC layout of BCH8 uses 14 bytes for 512 byte of data to make compatible
> > > > with RBL ECC layout, even though the requirement was only 13 byte. This
> > > > results a common ecc layout across RBL, U-boot & Linux.
> > > > 
> > > 
> > > See a few comments below,
> > > 
> > > (...)
> > > > +static int omap_elm_correct_data(struct mtd_info *mtd, u_char *dat,
> > > > +                               u_char *read_ecc, u_char *calc_ecc)
> > > > +{
> > > > +       struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> > > > +                       mtd);
> > > > +       int eccsteps = info->nand.ecc.steps;
> > > > +       int i , j, stat = 0;
> > > > +       int eccsize, eccflag, size;
> > > > +       struct elm_errorvec err_vec[ERROR_VECTOR_MAX];
> > > > +       u_char *ecc_vec = calc_ecc;
> > > > +       enum bch_ecc type;
> > > > +       bool is_error_reported = false;
> > > > +
> > > > +       /* initialize elm error vector to zero */
> > > > +       memset(err_vec, 0, sizeof(err_vec));
> > > > +       if (info->nand.ecc.strength == BCH8_MAX_ERROR) {
> > > > +               size = BCH8_SIZE;
> > > > +               eccsize = BCH8_ECC_OOB_BYTES;
> > > > +               type = BCH8_ECC;
> > > > +       } else {
> > > > +               size = BCH4_SIZE;
> > > > +               eccsize = BCH4_SIZE;
> > > > +               type = BCH4_ECC;
> > > > +       }
> > > > +
> > > > +       for (i = 0; i < eccsteps ; i++) {
> > > > +               eccflag = 0;    /* initialize eccflag */
> > > > +
> > > > +               for (j = 0; (j < eccsize); j++) {
> > > > +                       if (read_ecc[j] != 0xFF) {
> > > > +                               eccflag = 1;    /* data area is flashed */
> > > 
> > > Just a reminder: this way of checking if a page has been programmed is not robust to bitflips,
> > > so you may get into trouble with UBIFS on a fairly recent device.
> 
> Sorry I missed this point.
> 
> Here we were checking data in spare area (only in ecc layout 14*4 for BCH8) is 0xFF. If all data
> in spare area is 0xFF, conclude that the page is erased & no need of error correction. In case
> of bit flip present in spare area, page will be reported as uncorrectable.
> But I am not understand understand " trouble with UBIFS on a fairly recent device".
> Can you little elaborativ

There are at least 2 potential problems when reading an erased page with bitflips:

1. bitflip in data area and no bitflip in spare area (all 0xff)
Your code will not perform any ECC correction.
UBIFS does not like finding bitflips in empty pages, see for instance
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040328.html.

2. bitflip in ECC bytes in spare area
Your code will report an uncorrectable error upon reading; if this happens while reading a partially programmed UBI block,
I guess you will lose data.

BR,
--
Ivan

RE: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Philip, Avinash]

On Fri, Oct 05, 2012 at 19:53:38, Ivan Djelic wrote:
> On Fri, Oct 05, 2012 at 09:51:50AM +0100, Philip, Avinash wrote:
> > On Thu, Oct 04, 2012 at 15:51:03, Philip, Avinash wrote:
> > > On Thu, Oct 04, 2012 at 00:50:45, Ivan Djelic wrote:
> > > > On Wed, Oct 03, 2012 at 03:29:49PM +0100, Philip, Avinash wrote:
> > > > > ELM module can be used for error correction of BCH 4 & 8 bit. Also
> > > > > support read & write page in one shot by adding custom read_page &
> > > > > write_page methods. This helps in optimizing code.
> > > > > 
> > > > > New structure member "is_elm_used" is added to know the status of
> > > > > whether the ELM module is used for error correction or not.
> > > > > 
> > > > > Note:
> > > > > ECC layout of BCH8 uses 14 bytes for 512 byte of data to make compatible
> > > > > with RBL ECC layout, even though the requirement was only 13 byte. This
> > > > > results a common ecc layout across RBL, U-boot & Linux.
> > > > > 
> > > > 
> > > > See a few comments below,
> > > > 
> > > > (...)
> > > > > +static int omap_elm_correct_data(struct mtd_info *mtd, u_char *dat,
> > > > > +                               u_char *read_ecc, u_char *calc_ecc)
> > > > > +{
> > > > > +       struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> > > > > +                       mtd);
> > > > > +       int eccsteps = info->nand.ecc.steps;
> > > > > +       int i , j, stat = 0;
> > > > > +       int eccsize, eccflag, size;
> > > > > +       struct elm_errorvec err_vec[ERROR_VECTOR_MAX];
> > > > > +       u_char *ecc_vec = calc_ecc;
> > > > > +       enum bch_ecc type;
> > > > > +       bool is_error_reported = false;
> > > > > +
> > > > > +       /* initialize elm error vector to zero */
> > > > > +       memset(err_vec, 0, sizeof(err_vec));
> > > > > +       if (info->nand.ecc.strength == BCH8_MAX_ERROR) {
> > > > > +               size = BCH8_SIZE;
> > > > > +               eccsize = BCH8_ECC_OOB_BYTES;
> > > > > +               type = BCH8_ECC;
> > > > > +       } else {
> > > > > +               size = BCH4_SIZE;
> > > > > +               eccsize = BCH4_SIZE;
> > > > > +               type = BCH4_ECC;
> > > > > +       }
> > > > > +
> > > > > +       for (i = 0; i < eccsteps ; i++) {
> > > > > +               eccflag = 0;    /* initialize eccflag */
> > > > > +
> > > > > +               for (j = 0; (j < eccsize); j++) {
> > > > > +                       if (read_ecc[j] != 0xFF) {
> > > > > +                               eccflag = 1;    /* data area is flashed */
> > > > 
> > > > Just a reminder: this way of checking if a page has been programmed is not robust to bitflips,
> > > > so you may get into trouble with UBIFS on a fairly recent device.
> > 
> > Sorry I missed this point.
> > 
> > Here we were checking data in spare area (only in ecc layout 14*4 for BCH8) is 0xFF. If all data
> > in spare area is 0xFF, conclude that the page is erased & no need of error correction. In case
> > of bit flip present in spare area, page will be reported as uncorrectable.
> > But I am not understand understand " trouble with UBIFS on a fairly recent device".
> > Can you little elaborativ
> 
> There are at least 2 potential problems when reading an erased page with bitflips:
> 
> 1. bitflip in data area and no bitflip in spare area (all 0xff)
> Your code will not perform any ECC correction.
> UBIFS does not like finding bitflips in empty pages, see for instance
> http://lists.infradead.org/pipermail/linux-mtd/2012-March/040328.html.

In case of error correction using ELM, syndrome vector calculated after reading
Data area & OOB area. So handling of erased page requires a software workaround.
I am planning something as follows.

I will first check calculated ecc, which would be zero for non error pages.
Then I would check 0xFF in OOB area (for erased page) by checking number of
bit zeros in OOB area.  If it is 0xFF (number of bit zero count is zero),
set entire page as 0xFF if number of bit zeros is less than max bit flips
(8 or 4) by counting the number of bit zero's in data area.

This logic is implemented in fsmc_nand.c

See commit
mtd: fsmc: Newly erased page read algorithm implemented

> 
> 2. bitflip in ECC bytes in spare area
> Your code will report an uncorrectable error upon reading; if this happens while reading a partially programmed UBI block,
> I guess you will lose data.

In case of uncorrectable errors due to bit flips in spare area,
I can go on checking number of bit zero's in data area + OOB area
are less than max bit flips (8 or 4), I can go on setting the entire
page as 0xFF.

Thanks
Avinash

> 
> BR,
> --
> Ivan
> 

Re: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Ivan Djelic]

On Tue, Oct 09, 2012 at 01:36:50PM +0100, Philip, Avinash wrote:
(...)
> > There are at least 2 potential problems when reading an erased page with bitflips:
> > 
> > 1. bitflip in data area and no bitflip in spare area (all 0xff)
> > Your code will not perform any ECC correction.
> > UBIFS does not like finding bitflips in empty pages, see for instance
> > http://lists.infradead.org/pipermail/linux-mtd/2012-March/040328.html.
> 
> In case of error correction using ELM, syndrome vector calculated after reading
> Data area & OOB area. So handling of erased page requires a software workaround.
> I am planning something as follows.
> 
> I will first check calculated ecc, which would be zero for non error pages.
> Then I would check 0xFF in OOB area (for erased page) by checking number of
> bit zeros in OOB area.  If it is 0xFF (number of bit zero count is zero),
> set entire page as 0xFF if number of bit zeros is less than max bit flips
> (8 or 4) by counting the number of bit zero's in data area.
> 
> This logic is implemented in fsmc_nand.c
> 
> See commit
> mtd: fsmc: Newly erased page read algorithm implemented
> 
> > 
> > 2. bitflip in ECC bytes in spare area
> > Your code will report an uncorrectable error upon reading; if this happens while reading a partially programmed UBI block,
> > I guess you will lose data.
> 
> In case of uncorrectable errors due to bit flips in spare area,
> I can go on checking number of bit zero's in data area + OOB area
> are less than max bit flips (8 or 4), I can go on setting the entire
> page as 0xFF.
> 

OK, sounds reasonable.
Another simple strategy could use the fact that you add a 14th zero byte to
the 13 BCH bytes for RBL compatibility:

Upon reading:
 - if this 14th byte is zero (*) => page was programmed: perform ECC
   correction as usual
 - else, page was not programmed: do not perform ECC, read entire data+spare
   area, and set it to 0xff if less than 8 or 4 (max bitflips) zero bits
   were found

(*) for robustness to bitflip in 14th byte, replace condition
"14th byte is zero" by e.g. "14th byte has less than 4 bits set to 1".

What do you think ?

BR,
--
Ivan

RE: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Philip, Avinash]

On Wed, Oct 10, 2012 at 22:38:06, Ivan Djelic wrote:
> On Tue, Oct 09, 2012 at 01:36:50PM +0100, Philip, Avinash wrote:
> (...)
> > > There are at least 2 potential problems when reading an erased page with bitflips:
> > > 
> > > 1. bitflip in data area and no bitflip in spare area (all 0xff)
> > > Your code will not perform any ECC correction.
> > > UBIFS does not like finding bitflips in empty pages, see for instance
> > > http://lists.infradead.org/pipermail/linux-mtd/2012-March/040328.html.
> > 
> > In case of error correction using ELM, syndrome vector calculated after reading
> > Data area & OOB area. So handling of erased page requires a software workaround.
> > I am planning something as follows.
> > 
> > I will first check calculated ecc, which would be zero for non error pages.
> > Then I would check 0xFF in OOB area (for erased page) by checking number of
> > bit zeros in OOB area.  If it is 0xFF (number of bit zero count is zero),
> > set entire page as 0xFF if number of bit zeros is less than max bit flips
> > (8 or 4) by counting the number of bit zero's in data area.
> > 
> > This logic is implemented in fsmc_nand.c
> > 
> > See commit
> > mtd: fsmc: Newly erased page read algorithm implemented
> > 
> > > 
> > > 2. bitflip in ECC bytes in spare area
> > > Your code will report an uncorrectable error upon reading; if this happens while reading a partially programmed UBI block,
> > > I guess you will lose data.
> > 
> > In case of uncorrectable errors due to bit flips in spare area,
> > I can go on checking number of bit zero's in data area + OOB area
> > are less than max bit flips (8 or 4), I can go on setting the entire
> > page as 0xFF.
> > 
> 
> OK, sounds reasonable.
> Another simple strategy could use the fact that you add a 14th zero byte to
> the 13 BCH bytes for RBL compatibility:

RBL compatibility (14th byte) is applicable only for BCH8 ecc scheme.

So I am planning adding an extra byte (0) for BCH4 ecc scheme. So with this
we can go for same approaches in BCH4 & BCH8 ecc scheme.

If I understood correctly, software BCH ecc scheme is modifying calculated
ecc data to handle bit flips in erased pages.

If that is the only reason, whether same logic can go for same ECC calculation
(remove modification of calculated ecc in case of software ecc correction)
by adding an extra byte (0) in spare area to handle erased pages.

So can you share if I am missing something?

> 
> Upon reading:
>  - if this 14th byte is zero (*) => page was programmed: perform ECC
>    correction as usual
>  - else, page was not programmed: do not perform ECC, read entire data+spare
>    area, and set it to 0xff if less than 8 or 4 (max bitflips) zero bits
>    were found
> 
> (*) for robustness to bitflip in 14th byte, replace condition
> "14th byte is zero" by e.g. "14th byte has less than 4 bits set to 1".
> 
> What do you think ?

This seems logically good.

Thanks
Avinash

> 
> BR,
> --
> Ivan
> 

Re: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Ivan Djelic]

On Thu, Oct 11, 2012 at 06:27:13AM +0100, Philip, Avinash wrote:
(...)
> > Another simple strategy could use the fact that you add a 14th zero byte to
> > the 13 BCH bytes for RBL compatibility:
> 
> RBL compatibility (14th byte) is applicable only for BCH8 ecc scheme.
> 
> So I am planning adding an extra byte (0) for BCH4 ecc scheme. So with this
> we can go for same approaches in BCH4 & BCH8 ecc scheme.
> 
> If I understood correctly, software BCH ecc scheme is modifying calculated
> ecc data to handle bit flips in erased pages.
> 
> If that is the only reason, whether same logic can go for same ECC calculation
> (remove modification of calculated ecc in case of software ecc correction)
> by adding an extra byte (0) in spare area to handle erased pages.
> 
> So can you share if I am missing something?

Yes, the only reason why a constant polynomial is added to hw-generated ECC bytes is to transparently handle bitflips in erased pages.
Handling erased pages this way has several benefits over the zero byte hack:
- cleaner code, no checking of the zero byte
- no expensive scan of data+spare area when reading an erased block: this step can significantly slow down the initial UBI scan (lots of erased pages to read)
- no need to worry about the (very unlikely) possibility of having more than 4 bitflips in the zero byte

OTOH, having the same ECC codes for both ELM and non-ELM chips with RBL compatibility sounds nice and would also simplify things.
Note: on platforms where we use SW BCH correction, we also use the MLC OMAP boot mode, which is more robust and not compatible with 8-bit/4-bit BCH layouts.

I don't know which way is better for the OMAP community:
1. Unifying ECC modes = loosing the constant polynomial benefits, but gaining RBL compat and simplifying code
2. Keeping separate ECC modes = code bloat

Tony, do you have an opinion on this ?

BTW, Afzal is submitting a series of patches [1] which are not compatible with your series; is there any plan to merge your patches ?

BR,
--
Ivan

[1] http://lists.infradead.org/pipermail/linux-mtd/2012-October/044374.html

RE: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Philip, Avinash]

On Thu, Oct 11, 2012 at 13:51:49, Ivan Djelic wrote:
> On Thu, Oct 11, 2012 at 06:27:13AM +0100, Philip, Avinash wrote:
> (...)
> > > Another simple strategy could use the fact that you add a 14th zero byte to
> > > the 13 BCH bytes for RBL compatibility:
> > 
> > RBL compatibility (14th byte) is applicable only for BCH8 ecc scheme.
> > 
> > So I am planning adding an extra byte (0) for BCH4 ecc scheme. So with this
> > we can go for same approaches in BCH4 & BCH8 ecc scheme.
> > 
> > If I understood correctly, software BCH ecc scheme is modifying calculated
> > ecc data to handle bit flips in erased pages.
> > 
> > If that is the only reason, whether same logic can go for same ECC calculation
> > (remove modification of calculated ecc in case of software ecc correction)
> > by adding an extra byte (0) in spare area to handle erased pages.
> > 
> > So can you share if I am missing something?
> 
> Yes, the only reason why a constant polynomial is added to hw-generated ECC
> bytes is to transparently handle bitflips in erased pages.
> Handling erased pages this way has several benefits over the zero byte hack:
> - cleaner code, no checking of the zero byte
> - no expensive scan of data+spare area when reading an erased block: this
>   step can significantly slow down the initial UBI scan (lots of erased
>    pages to read)

Thanks for raising this point.
In order to reduce scan time of data+spare area when reading an erased block,
we can follow below steps

1. Create a standard ecc vector for erased page & check against it with calculated
   ecc of erased page.
2. If the vectors won't match, go for counting bit flips in erased page.
3. Again filtering of erased page can done by checking zero at fixed locations
   in spare area.
Note:
Reading of bits in erased page should done only for pages having bitflips.

> - no need to worry about the (very unlikely) possibility of having more
>   than 4 bitflips in the zero byte
> 
> OTOH, having the same ECC codes for both ELM and non-ELM chips with RBL
> compatibility sounds nice and would also simplify things.
> Note: on platforms where we use SW BCH correction, we also use the
> MLC OMAP boot mode, which is more robust and not compatible
> with 8-bit/4-bit BCH layouts.
> 
> I don't know which way is better for the OMAP community:
> 1. Unifying ECC modes = loosing the constant polynomial benefits,
>    but gaining RBL compat and simplifying code
> 2. Keeping separate ECC modes = code bloat
> 
> Tony, do you have an opinion on this ?
> 
> BTW, Afzal is submitting a series of patches [1] which are not
> compatible with your series; is there any plan to merge your patches ?

My next series will be on top Afzal's changes.

Thanks
Avinash

> 
> BR,
> --
> Ivan
> 
> [1] http://lists.infradead.org/pipermail/linux-mtd/2012-October/044374.html
> 

Re: [PATCH 4/4] mtd: nand: omap2: Add data correction support

[Tony Lindgren]

* Ivan Djelic <ivan.djelic@parrot.com> [121011 01:23]:
> 
> I don't know which way is better for the OMAP community:
> 1. Unifying ECC modes = loosing the constant polynomial benefits, but gaining RBL compat and simplifying code
> 2. Keeping separate ECC modes = code bloat
> 
> Tony, do you have an opinion on this ?

Well right now I'm mostly interested in making device
drivers independent of the arch/arm/*omap*/* code.
That's where Afzal's patches help quite a bit, so let's
get those in first. So from that point of view, option #1
above sounds better as the first step :)

Ideally of course option #1 should not limit us from
adding extra features in the long run.

Regards,

Tony