RE: [PATCH v4 3/4] mtd: nand: omap: add support for BCH16_ECC - NAND driver updates
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
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
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 --- 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)