Patchwork [v2] ARM: OMAP3: gpmc: add BCH ecc api and modes

login
register
mail settings
Submitter Ivan Djelic
Date April 19, 2012, 6:43 p.m.
Message ID <1334861024-27386-1-git-send-email-ivan.djelic@parrot.com>
Download mbox | patch
Permalink /patch/153850/
State New
Headers show

Comments

Ivan Djelic - April 19, 2012, 6:43 p.m.
Hello,
Here is version 2 of this patch, fixing in bug discovered while testing
on a BeagleBoard rev C3 (OMAP3530 ES3.0 + Micron NAND 256MiB 1,8V 16-bit).
--
Ivan


This patch adds a simple BCH ecc computation api, similar to the
existing Hamming ecc api. It is intended to be used by the MTD layer.
It implements the following features:

- support 4-bit and 8-bit ecc computation
- do not protect user bytes in spare area, only data area is protected
- ecc for an erased NAND page (0xFFs) is also a sequence of 0xFFs

This last feature is obtained by adding a constant polynomial to
the hardware computed ecc. It allows to correct bitflips in blank pages
and is extremely useful to support filesystems such as UBIFS, which expect
erased pages to contain only 0xFFs.

This api has been tested on an OMAP3630 board.

Signed-off-by: Ivan Djelic <ivan.djelic@parrot.com>
---
 v2 changelog: added missing control register configuration

 arch/arm/mach-omap2/gpmc.c             |  148 ++++++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/gpmc.h |   10 +++
 2 files changed, 158 insertions(+)
Tony Lindgren - April 25, 2012, 5:03 p.m.
Hi,

Few comments below.

* Ivan Djelic <ivan.djelic@parrot.com> [120419 11:49]:
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 00d5108..e3a91a1 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -49,6 +49,7 @@
>  #define GPMC_ECC_CONTROL	0x1f8
>  #define GPMC_ECC_SIZE_CONFIG	0x1fc
>  #define GPMC_ECC1_RESULT        0x200
> +#define GPMC_ECC_BCH_RESULT_0   0x240

Can you please add a comment here saying something like:

#define GPMC_ECC_BCH_RESULT_0   0x240	/* Not available on omap2 */
  

> @@ -920,3 +921,150 @@ int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpmc_calculate_ecc);
> +
> +#ifdef CONFIG_ARCH_OMAP3
> +
> +/**
> + * gpmc_enable_hwecc_bch - enable hardware BCH ecc functionality
> + * @cs: chip select number
> + * @mode: read/write mode
> + * @dev_width: device bus width(1 for x16, 0 for x8)
> + * @nsectors: how many 512-byte sectors to process
> + * @nerrors: how many errors to correct per sector (4 or 8)
> + */
> +int gpmc_enable_hwecc_bch(int cs, int mode, int dev_width, int nsectors,
> +			  int nerrors)
> +{
> +	unsigned int val;
> +
> +	/* check if ecc module is in use */
> +	if (gpmc_ecc_used != -EINVAL)
> +		return -EINVAL;
> +	/*
> +	 * FIXME: some OMAP3 revisions have a hardware bug which prevents
> +	 * the 4-bit BCH mode from working properly. Such revisions could be
> +	 * detected and rejected here.
> +	 */

This should then be disabled to avoid corruption. Maybe only allow it
initially on omaps that have been tested? And for omap2 it should return
error  for sure.

Or do you know the broken omap3 versions?

Also, should you first request this feature in case multiple drivers
need to share it?

Other than that, looks good to me.

Regards,

Tony
Ivan Djelic - April 25, 2012, 6:28 p.m.
Hi Tony,
Thanks for the review,

On Wed, Apr 25, 2012 at 06:03:14PM +0100, Tony Lindgren wrote:
(...)
> >  #define GPMC_ECC1_RESULT        0x200
> > +#define GPMC_ECC_BCH_RESULT_0   0x240
> 
> Can you please add a comment here saying something like:
> 
> #define GPMC_ECC_BCH_RESULT_0   0x240	/* Not available on omap2 */

OK sure.

> > +	/* check if ecc module is in use */
> > +	if (gpmc_ecc_used != -EINVAL)
> > +		return -EINVAL;
> > +	/*
> > +	 * FIXME: some OMAP3 revisions have a hardware bug which prevents
> > +	 * the 4-bit BCH mode from working properly. Such revisions could be
> > +	 * detected and rejected here.
> > +	 */
> 
> This should then be disabled to avoid corruption. Maybe only allow it
> initially on omaps that have been tested? And for omap2 it should return
> error  for sure.

OK I'll add a check.

> 
> Or do you know the broken omap3 versions?

Well, I was hoping that someone from linux-omap could tell me :)
I found this HW ECC feature table in
http://processors.wiki.ti.com/index.php/Raw_NAND_ECC:

         1b     4b      8b
---------------------------
OMAP35x  YES    NO      YES
AM35x    YES    YES     YES
AM/DM37x YES    YES     YES

and other wiki pages confirmed that 4-bit mode is not supported on all OMAP35xx chips.
OTOH, I know from TI support that 4-bit mode is at least supported on
OMAP3630 ES1.x (x >= 1).

So, a conservative approach would be to reject 4-bit mode on all chips but
omap3630 with rev >= 1.1. Other revisions/chips could be added later if they are
confirmed to work; what do you think ?

> Also, should you first request this feature in case multiple drivers
> need to share it?

According to TI documentation (OMAP36xx ES1.x TRM, §10.1.4, GPMC functional diagram),
the GPMC ECC engines (Hamming and BCH) are dedicated to NAND access only; therefore
I believe the mtd driver is the only potential user of this feature.
Also, the existing Hamming ecc API does not perform any request; or did I miss
something? If I need to perform the request, is there an existing api to do so?

Thanks,
--
Ivan
Tony Lindgren - April 25, 2012, 8:57 p.m.
* Ivan Djelic <ivan.djelic@parrot.com> [120425 11:33]:
> Hi Tony,
> Thanks for the review,
> 
> On Wed, Apr 25, 2012 at 06:03:14PM +0100, Tony Lindgren wrote:
> (...)
> > >  #define GPMC_ECC1_RESULT        0x200
> > > +#define GPMC_ECC_BCH_RESULT_0   0x240
> > 
> > Can you please add a comment here saying something like:
> > 
> > #define GPMC_ECC_BCH_RESULT_0   0x240	/* Not available on omap2 */
> 
> OK sure.
> 
> > > +	/* check if ecc module is in use */
> > > +	if (gpmc_ecc_used != -EINVAL)
> > > +		return -EINVAL;
> > > +	/*
> > > +	 * FIXME: some OMAP3 revisions have a hardware bug which prevents
> > > +	 * the 4-bit BCH mode from working properly. Such revisions could be
> > > +	 * detected and rejected here.
> > > +	 */
> > 
> > This should then be disabled to avoid corruption. Maybe only allow it
> > initially on omaps that have been tested? And for omap2 it should return
> > error  for sure.
> 
> OK I'll add a check.
> 
> > 
> > Or do you know the broken omap3 versions?
> 
> Well, I was hoping that someone from linux-omap could tell me :)
> I found this HW ECC feature table in
> http://processors.wiki.ti.com/index.php/Raw_NAND_ECC:
> 
>          1b     4b      8b
> ---------------------------
> OMAP35x  YES    NO      YES
> AM35x    YES    YES     YES
> AM/DM37x YES    YES     YES
> 
> and other wiki pages confirmed that 4-bit mode is not supported on all OMAP35xx chips.
> OTOH, I know from TI support that 4-bit mode is at least supported on
> OMAP3630 ES1.x (x >= 1).
> 
> So, a conservative approach would be to reject 4-bit mode on all chips but
> omap3630 with rev >= 1.1. Other revisions/chips could be added later if they are
> confirmed to work; what do you think ?

Sounds good to me.
 
> > Also, should you first request this feature in case multiple drivers
> > need to share it?
> 
> According to TI documentation (OMAP36xx ES1.x TRM, §10.1.4, GPMC functional diagram),
> the GPMC ECC engines (Hamming and BCH) are dedicated to NAND access only; therefore
> I believe the mtd driver is the only potential user of this feature.
> Also, the existing Hamming ecc API does not perform any request; or did I miss
> something? If I need to perform the request, is there an existing api to do so?

OK I guess the only conflict would be multiple NAND chips then, which we don't
have at least currently AFAIK.

Tony

Patch

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 00d5108..e3a91a1 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -49,6 +49,7 @@ 
 #define GPMC_ECC_CONTROL	0x1f8
 #define GPMC_ECC_SIZE_CONFIG	0x1fc
 #define GPMC_ECC1_RESULT        0x200
+#define GPMC_ECC_BCH_RESULT_0   0x240
 
 #define GPMC_CS0_OFFSET		0x60
 #define GPMC_CS_SIZE		0x30
@@ -920,3 +921,150 @@  int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(gpmc_calculate_ecc);
+
+#ifdef CONFIG_ARCH_OMAP3
+
+/**
+ * gpmc_enable_hwecc_bch - enable hardware BCH ecc functionality
+ * @cs: chip select number
+ * @mode: read/write mode
+ * @dev_width: device bus width(1 for x16, 0 for x8)
+ * @nsectors: how many 512-byte sectors to process
+ * @nerrors: how many errors to correct per sector (4 or 8)
+ */
+int gpmc_enable_hwecc_bch(int cs, int mode, int dev_width, int nsectors,
+			  int nerrors)
+{
+	unsigned int val;
+
+	/* check if ecc module is in use */
+	if (gpmc_ecc_used != -EINVAL)
+		return -EINVAL;
+	/*
+	 * FIXME: some OMAP3 revisions have a hardware bug which prevents
+	 * the 4-bit BCH mode from working properly. Such revisions could be
+	 * detected and rejected here.
+	 */
+
+	gpmc_ecc_used = cs;
+
+	/* 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));
+
+	/* BCH configuration */
+	val = ((1                        << 16) | /* enable BCH */
+	       (((nerrors == 8) ? 1 : 0) << 12) | /* 8 or 4 bits */
+	       (0x06                     <<  8) | /* wrap mode = 6 */
+	       (dev_width                <<  7) | /* bus width */
+	       (((nsectors-1) & 0x7)     <<  4) | /* number of sectors */
+	       (cs                       <<  1) | /* ECC CS */
+	       (0x1));                            /* enable ECC */
+
+	gpmc_write_reg(GPMC_ECC_CONFIG, val);
+	gpmc_write_reg(GPMC_ECC_CONTROL, 0x101);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpmc_enable_hwecc_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
+ * @ecc: The ecc output buffer
+ */
+int gpmc_calculate_ecc_bch4(int cs, const u_char *dat, u_char *ecc)
+{
+	int i;
+	unsigned long nsectors, reg, val1, val2;
+
+	if (gpmc_ecc_used != cs)
+		return -EINVAL;
+
+	nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
+
+	for (i = 0; i < nsectors; i++) {
+
+		reg = GPMC_ECC_BCH_RESULT_0 + 16*i;
+
+		/* Read hw-computed remainder */
+		val1 = gpmc_read_reg(reg + 0);
+		val2 = gpmc_read_reg(reg + 4);
+
+		/*
+		 * Add constant polynomial to remainder, in order to get an ecc
+		 * sequence of 0xFFs for a buffer filled with 0xFFs; and
+		 * left-justify the resulting polynomial.
+		 */
+		*ecc++ = 0x28 ^ ((val2 >> 12) & 0xFF);
+		*ecc++ = 0x13 ^ ((val2 >>  4) & 0xFF);
+		*ecc++ = 0xcc ^ (((val2 & 0xF) << 4)|((val1 >> 28) & 0xF));
+		*ecc++ = 0x39 ^ ((val1 >> 20) & 0xFF);
+		*ecc++ = 0x96 ^ ((val1 >> 12) & 0xFF);
+		*ecc++ = 0xac ^ ((val1 >> 4) & 0xFF);
+		*ecc++ = 0x7f ^ ((val1 & 0xF) << 4);
+	}
+
+	gpmc_ecc_used = -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch4);
+
+/**
+ * gpmc_calculate_ecc_bch8 - Generate 13 ecc bytes per block of 512 data bytes
+ * @cs:  chip select number
+ * @dat: The pointer to data on which ecc is computed
+ * @ecc: The ecc output buffer
+ */
+int gpmc_calculate_ecc_bch8(int cs, const u_char *dat, u_char *ecc)
+{
+	int i;
+	unsigned long nsectors, reg, val1, val2, val3, val4;
+
+	if (gpmc_ecc_used != cs)
+		return -EINVAL;
+
+	nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
+
+	for (i = 0; i < nsectors; i++) {
+
+		reg = GPMC_ECC_BCH_RESULT_0 + 16*i;
+
+		/* Read hw-computed remainder */
+		val1 = gpmc_read_reg(reg + 0);
+		val2 = gpmc_read_reg(reg + 4);
+		val3 = gpmc_read_reg(reg + 8);
+		val4 = gpmc_read_reg(reg + 12);
+
+		/*
+		 * Add constant polynomial to remainder, in order to get an ecc
+		 * sequence of 0xFFs for a buffer filled with 0xFFs.
+		 */
+		*ecc++ = 0xef ^ (val4 & 0xFF);
+		*ecc++ = 0x51 ^ ((val3 >> 24) & 0xFF);
+		*ecc++ = 0x2e ^ ((val3 >> 16) & 0xFF);
+		*ecc++ = 0x09 ^ ((val3 >> 8) & 0xFF);
+		*ecc++ = 0xed ^ (val3 & 0xFF);
+		*ecc++ = 0x93 ^ ((val2 >> 24) & 0xFF);
+		*ecc++ = 0x9a ^ ((val2 >> 16) & 0xFF);
+		*ecc++ = 0xc2 ^ ((val2 >> 8) & 0xFF);
+		*ecc++ = 0x97 ^ (val2 & 0xFF);
+		*ecc++ = 0x79 ^ ((val1 >> 24) & 0xFF);
+		*ecc++ = 0xe5 ^ ((val1 >> 16) & 0xFF);
+		*ecc++ = 0x24 ^ ((val1 >> 8) & 0xFF);
+		*ecc++ = 0xb5 ^ (val1 & 0xFF);
+	}
+
+	gpmc_ecc_used = -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch8);
+
+#endif /* CONFIG_ARCH_OMAP3 */
diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
index 1527929..23791ca 100644
--- a/arch/arm/plat-omap/include/plat/gpmc.h
+++ b/arch/arm/plat-omap/include/plat/gpmc.h
@@ -92,6 +92,8 @@  enum omap_ecc {
 	OMAP_ECC_HAMMING_CODE_HW, /* gpmc to detect the error */
 		/* 1-bit ecc: stored at beginning of spare area as romcode */
 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE, /* gpmc method & romcode layout */
+	OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */
+	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
 };
 
 /*
@@ -157,4 +159,12 @@  extern int gpmc_nand_write(int cs, int cmd, int wval);
 
 int gpmc_enable_hwecc(int cs, int mode, int dev_width, int ecc_size);
 int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code);
+
+#ifdef CONFIG_ARCH_OMAP3
+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);
+#endif /* CONFIG_ARCH_OMAP3 */
+
 #endif