RE: [PATCH v4 3/4] mtd: nand: omap: add support for BCH16_ECC - NAND driver updates

2014-05-22 Thread Gupta, Pekon 
Hi Brian,

From: Brian Norris
On Mon, May 19, 2014 at 01:24:41PM +0530, Pekon Gupta wrote:
 --- a/drivers/mtd/nand/omap2.c
 +++ b/drivers/mtd/nand/omap2.c
 @@ -1201,6 +1219,41 @@ static int __maybe_unused 
 omap_calculate_ecc_bch(struct mtd_info
*mtd,
  *ecc_code++ = ((bch_val1  4)  0xFF);
  *ecc_code++ = ((bch_val1  0xF)  4);
  break;
 +case OMAP_ECC_BCH16_CODE_HW:
 +val = readl(gpmc_regs-gpmc_bch_result6[i]);

For all of these 'gpmc_bch_resultX' fields, couldn't you make this into
a 2-D array? So to access BCH result 6 at sector i, it would be:

   val = readl(gpmc_regs-gpmc_bch_result[6][i];

This could help you to rewrite some of this stuff as loops, instead of
giant blocks of copy-paste-modify.


Thanks for excepting the series. With this series OMAP NAND driver
comes to a stable point supporting NAND boot on all latest platforms.

I had earlier experimented with something similar to your suggestions
but these clean-ups require update in multiple drivers (GPMC, NAND, ...),
and it was becoming messy, so I dropped it. There are some practical
challenges to this type of clean-up like;

(1) GPMC register addresses here gpmc_regs are initialized in GPMC driver
 So your suggested changes will affect multiple subsystems, hence
This type of clean-up is bit deferred from sometime.


(2) The register space for gpmc_bch_results is not contiguous.
GPMC_BCH_RESULT_0   RW  0x240-0x2B0
GPMC_BCH_RESULT_1   RW  0x244-0x2B4
GPMC_BCH_RESULT_2   RW  0x248-0x2B8
GPMC_BCH_RESULT_3   RW  0x24C-0x2BC
[...]
GPMC_BCH_RESULT_4   RW  0x300-0x370
GPMC_BCH_RESULT_5   RW  0x304-0x374
GPMC_BCH_RESULT_6   RW  0x308-0x378

This is because older version of GPMC hardware IP  (like in OMAP3)
supported only till BCH8 ecc-scheme. Later same hardware IP was
extended to support BCH16. So newer platforms have extended
register-map. So it was felt that instead of having separate for..loops
lets continue with replicating all 7 GPMC_BCH_RESULT registers.


 +ecc_code[0]  = ((val   8)  0xFF);
 +ecc_code[1]  = ((val   0)  0xFF);
 +val = readl(gpmc_regs-gpmc_bch_result5[i]);
 +ecc_code[2]  = ((val  24)  0xFF);
 +ecc_code[3]  = ((val  16)  0xFF);
 +ecc_code[4]  = ((val   8)  0xFF);
 +ecc_code[5]  = ((val   0)  0xFF);

A lot of this code can be rewritten to use the endian swapping macros, I
expect. Something like this looks equivalent:

   *((uint32_t *)ecc_code[2]) = cpu_to_be32(val);

You could probably fix the types up to make this look a little nicer.


(3) BCH4 use different alignment than BCH8 and BCH16 as ECC syndrome
is not bytewise aligned (it's of 6 1/2 bytes = 13 nibbles).  So for BCH4
higher-nibble or reg1 is shifted and ORed with lower-nibble of reg2.
There is no byte-to-byte mapping. So generic implementation becomes messy.

However, Roger Quadros is planning GPMC driver clean-up, so looping him
in-case he can incorporate some of these things while re-factoring GPMC code.


with regards, pekon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] mtd: nand: omap: add support for BCH16_ECC - NAND driver updates

2014-05-20 Thread Brian Norris 
On Mon, May 19, 2014 at 01:24:41PM +0530, Pekon Gupta wrote:
 --- a/drivers/mtd/nand/omap2.c
 +++ b/drivers/mtd/nand/omap2.c
 @@ -1201,6 +1219,41 @@ static int __maybe_unused 
 omap_calculate_ecc_bch(struct mtd_info *mtd,
   *ecc_code++ = ((bch_val1  4)  0xFF);
   *ecc_code++ = ((bch_val1  0xF)  4);
   break;
 + case OMAP_ECC_BCH16_CODE_HW:
 + val = readl(gpmc_regs-gpmc_bch_result6[i]);

For all of these 'gpmc_bch_resultX' fields, couldn't you make this into
a 2-D array? So to access BCH result 6 at sector i, it would be:

val = readl(gpmc_regs-gpmc_bch_result[6][i];

This could help you to rewrite some of this stuff as loops, instead of
giant blocks of copy-paste-modify.

 + ecc_code[0]  = ((val   8)  0xFF);
 + ecc_code[1]  = ((val   0)  0xFF);
 + val = readl(gpmc_regs-gpmc_bch_result5[i]);
 + ecc_code[2]  = ((val  24)  0xFF);
 + ecc_code[3]  = ((val  16)  0xFF);
 + ecc_code[4]  = ((val   8)  0xFF);
 + ecc_code[5]  = ((val   0)  0xFF);

A lot of this code can be rewritten to use the endian swapping macros, I
expect. Something like this looks equivalent:

*((uint32_t *)ecc_code[2]) = cpu_to_be32(val);

You could probably fix the types up to make this look a little nicer.

 + val = readl(gpmc_regs-gpmc_bch_result4[i]);
 + ecc_code[6]  = ((val  24)  0xFF);
 + ecc_code[7]  = ((val  16)  0xFF);
 + ecc_code[8]  = ((val   8)  0xFF);
 + ecc_code[9]  = ((val   0)  0xFF);
 + val = readl(gpmc_regs-gpmc_bch_result3[i]);
 + ecc_code[10] = ((val  24)  0xFF);
 + ecc_code[11] = ((val  16)  0xFF);
 + ecc_code[12] = ((val   8)  0xFF);
 + ecc_code[13] = ((val   0)  0xFF);
 + val = readl(gpmc_regs-gpmc_bch_result2[i]);
 + ecc_code[14] = ((val  24)  0xFF);
 + ecc_code[15] = ((val  16)  0xFF);
 + ecc_code[16] = ((val   8)  0xFF);
 + ecc_code[17] = ((val   0)  0xFF);
 + val = readl(gpmc_regs-gpmc_bch_result1[i]);
 + ecc_code[18] = ((val  24)  0xFF);
 + ecc_code[19] = ((val  16)  0xFF);
 + ecc_code[20] = ((val   8)  0xFF);
 + ecc_code[21] = ((val   0)  0xFF);
 + val = readl(gpmc_regs-gpmc_bch_result0[i]);
 + ecc_code[22] = ((val  24)  0xFF);
 + ecc_code[23] = ((val  16)  0xFF);
 + ecc_code[24] = ((val   8)  0xFF);
 + ecc_code[25] = ((val   0)  0xFF);
 + break;
   default:
   return -EINVAL;
   }

Brian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/4] mtd: nand: omap: add support for BCH16_ECC - NAND driver updates

2014-05-19 Thread Pekon Gupta 
This patch add support for BCH16 ecc-scheme in OMAP NAND driver, by extending
following functions:
 - omap_enable_hwecc (nand_chip-ecc.hwctl): configure GPMC controller
 - omap_calculate_ecc_bch (nand_chip-ecc.calculate): fetch ECC signature from 
GPMC controller
 - omap_elm_correct_data (nand_chip-ecc.correct): detect and correct ECC 
errors using ELM

(a) BCH16 ecc-scheme can detect and correct 16 bit-flips per 512Bytes of data.
(b) BCH16 ecc-scheme generates 26-bytes of ECC syndrome / 512B.
Due to (b) this scheme can only be used with NAND devices which have enough
OOB to satisfy the relation: OOBsize per page = 26 * (page-size / 512)

Signed-off-by: Pekon Gupta pe...@ti.com
---
 drivers/mtd/nand/omap2.c | 94 
 1 file changed, 94 insertions(+)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 0a68508..e0a6c82 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -137,6 +137,10 @@
 #define BADBLOCK_MARKER_LENGTH 2
 
 #ifdef CONFIG_MTD_NAND_OMAP_BCH
+static u_char bch16_vector[] = {0xf5, 0x24, 0x1c, 0xd0, 0x61, 0xb3, 0xf1, 0x55,
+   0x2e, 0x2c, 0x86, 0xa3, 0xed, 0x36, 0x1b, 0x78,
+   0x48, 0x76, 0xa9, 0x3b, 0x97, 0xd1, 0x7a, 0x93,
+   0x07, 0x0e};
 static u_char bch8_vector[] = {0xf3, 0xdb, 0x14, 0x16, 0x8b, 0xd2, 0xbe, 0xcc,
0xac, 0x6b, 0xff, 0x99, 0x7b};
 static u_char bch4_vector[] = {0x00, 0x6b, 0x31, 0xdd, 0x41, 0xbc, 0x10};
@@ -1114,6 +1118,19 @@ static void __maybe_unused omap_enable_hwecc_bch(struct 
mtd_info *mtd, int mode)
ecc_size1 = BCH_ECC_SIZE1;
}
break;
+   case OMAP_ECC_BCH16_CODE_HW:
+   bch_type = 0x2;
+   nsectors = chip-ecc.steps;
+   if (mode == NAND_ECC_READ) {
+   wr_mode   = 0x01;
+   ecc_size0 = 52; /* ECC bits in nibbles per sector */
+   ecc_size1 = 0;  /* non-ECC bits in nibbles per sector */
+   } else {
+   wr_mode   = 0x01;
+   ecc_size0 = 0;  /* extra bits in nibbles per sector */
+   ecc_size1 = 52; /* OOB bits in nibbles per sector */
+   }
+   break;
default:
return;
}
@@ -1162,6 +1179,7 @@ static int __maybe_unused omap_calculate_ecc_bch(struct 
mtd_info *mtd,
struct gpmc_nand_regs   *gpmc_regs = info-reg;
u8 *ecc_code;
unsigned long nsectors, bch_val1, bch_val2, bch_val3, bch_val4;
+   u32 val;
int i;
 
nsectors = ((readl(info-reg.gpmc_ecc_config)  4)  0x7) + 1;
@@ -1201,6 +1219,41 @@ static int __maybe_unused omap_calculate_ecc_bch(struct 
mtd_info *mtd,
*ecc_code++ = ((bch_val1  4)  0xFF);
*ecc_code++ = ((bch_val1  0xF)  4);
break;
+   case OMAP_ECC_BCH16_CODE_HW:
+   val = readl(gpmc_regs-gpmc_bch_result6[i]);
+   ecc_code[0]  = ((val   8)  0xFF);
+   ecc_code[1]  = ((val   0)  0xFF);
+   val = readl(gpmc_regs-gpmc_bch_result5[i]);
+   ecc_code[2]  = ((val  24)  0xFF);
+   ecc_code[3]  = ((val  16)  0xFF);
+   ecc_code[4]  = ((val   8)  0xFF);
+   ecc_code[5]  = ((val   0)  0xFF);
+   val = readl(gpmc_regs-gpmc_bch_result4[i]);
+   ecc_code[6]  = ((val  24)  0xFF);
+   ecc_code[7]  = ((val  16)  0xFF);
+   ecc_code[8]  = ((val   8)  0xFF);
+   ecc_code[9]  = ((val   0)  0xFF);
+   val = readl(gpmc_regs-gpmc_bch_result3[i]);
+   ecc_code[10] = ((val  24)  0xFF);
+   ecc_code[11] = ((val  16)  0xFF);
+   ecc_code[12] = ((val   8)  0xFF);
+   ecc_code[13] = ((val   0)  0xFF);
+   val = readl(gpmc_regs-gpmc_bch_result2[i]);
+   ecc_code[14] = ((val  24)  0xFF);
+   ecc_code[15] = ((val  16)  0xFF);
+   ecc_code[16] = ((val   8)  0xFF);
+   ecc_code[17] = ((val   0)  0xFF);
+   val = readl(gpmc_regs-gpmc_bch_result1[i]);
+   ecc_code[18] = ((val  24)  0xFF);
+   ecc_code[19] = ((val  16)  0xFF);
+   ecc_code[20] = ((val   8)  0xFF);
+   ecc_code[21] = ((val   0)  0xFF);
+   val = readl(gpmc_regs-gpmc_bch_result0[i]);
+   ecc_code[22] = ((val  24)  0xFF);
+   ecc_code[23] = ((val  16)  0xFF);
+   ecc_code[24] = ((val   8)  0xFF);
+