[PATCH v11 4/4] MTD: at91: atmel_nand: Update driver to support Programmable Multibit ECC controller (REPORT SPAM)

[William F.]

Em 26-06-2012 11:15, Richard Genoud escreveu:
> 2012/6/25 Josh Wu<josh.wu at atmel.com>:
>> +static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf,
>> +               int extra_bytes, int err_nbr)
>> +{
>> +       struct nand_chip *nand_chip = mtd->priv;
>> +       struct atmel_nand_host *host = nand_chip->priv;
>> +       int i = 0;
>> +       int byte_pos, bit_pos, sector_size, ecc_size;
>> +       uint32_t tmp;
>> +
>> +       sector_size = host->pmecc_sector_size;
>> +       ecc_size = nand_chip->ecc.bytes;
>> +
>> +       while (err_nbr) {
>> +               tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
>> +               byte_pos = tmp / 8;
>> +               bit_pos  = tmp % 8;
>> +               dev_info(host->dev, "PMECC correction, byte_pos: %d bit_pos: %d\n",
>> +                                       byte_pos, bit_pos);
>> +
>> +               if (byte_pos<  (sector_size + extra_bytes)) {
>> +                       tmp = sector_size +
>> +                               pmecc_readl_relaxed(host->ecc, SADDR);
>> +
>> +                       if (byte_pos<  tmp)
>> +                               *(buf + byte_pos) ^= (1<<  bit_pos);
>> +                       else
>> +                               *(buf + byte_pos + ecc_size) ^= (1<<  bit_pos);
>> +               }
> I think there's a problem in there.
> On my board (sam9g35-ek), I've got 2KB pages and 4 sectors per page + 64B OOB.
> When I flip a bit on the 63rd OOB byte (the last ECC byte), byte_pos is 515.
> If I flip a bit on the 51th OOB byte (the 3rd ECC byte), byte_pos is also 515.
> So, if I get it correct, byte_pos will always be between 0 and
> sector_size + sector_ecc_bytes.
> (and I think that sector_ecc_bytes == extra_bytes (cf below))
> And, pmecc_readl_relaxed(host->ecc, SADDR) gives me the ECC offset in OOB (48).
> So, (byte_pos<  sector_size + extra_bytes) will always be correct as
> far as the PMECC controller doesn't give us crap.
> But, IHMO the "tmp = sector_size + pmecc_readl_relaxed(host->ecc,
> SADDR);" line is wrong.
>
> I think that it should be something like this:
> static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
> 		int extra_bytes, int err_nbr, int sector_number)
> {
> [...]
> 		tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
> 		byte_pos = tmp / 8;
> 		bit_pos  = tmp % 8;
> 		if (byte_pos<  (sector_size + extra_bytes)) {
> 			if (byte_pos<  sector_size) {
> 				*(buf + byte_pos) ^= (1<<  bit_pos);
> 				/* It may be easier to read this info if the byte_pos
> 				 * is relative to the page start, not just the sector */
> 				dev_info(host->dev, "PMECC correction, byte_pos: %d bit_pos: %d\n",
> 					 sector_number * sector_sz + byte_pos, bit_pos);
> 			} else {
> 				/* number of ecc bytes per sector */
> 				tmp = pmecc_get_ecc_bytes(nand_chip->ecc.strength, sector_size);
> 				/* offset in the ecc of the ecc bytes for current sector */
> 				tmp *= sector_number;
> 				/* now, tmp is the offset of the byte error in the ecc */
> 				tmp += byte_pos - sector_size;
> 				ecc[tmp] ^= (1<<  bit_pos);
> 				dev_info(host->dev, "Bit flip in ECC, ecc_byte_pos: %d bit_pos: %d\n",
> 					 tmp, bit_pos);
> 				/* or */
> 				dev_info(host->dev, "Bit flip in OOB, oob_byte_pos: %d bit_pos: %d\n",
> 					 tmp + nand_chip->ecc_layout.eccpos[0], bit_pos);
> 			}
> 		}
> [...]
>
>> +
>> +               i++;
>> +               err_nbr--;
>> +       }
>> +
>> +       return;
>> +}
>> +
>> +static int pmecc_correction(struct mtd_info *mtd, u32 pmecc_stat, uint8_t *buf,
>> +       u8 *ecc)
>> +{
>> +       struct nand_chip *nand_chip = mtd->priv;
>> +       struct atmel_nand_host *host = nand_chip->priv;
>> +       int i, err_nbr, eccbytes;
>> +       uint8_t *buf_pos;
>> +
>> +       eccbytes = nand_chip->ecc.bytes;
>> +       for (i = 0; i<  eccbytes; i++)
>> +               if (ecc[i] != 0xff)
>> +                       goto normal_check;
>> +       /* Erased page, return OK */
>> +       return 0;
>> +
>> +normal_check:
>> +       for (i = 0; i<  host->pmecc_sector_number; i++) {
>> +               err_nbr = 0;
>> +               if (pmecc_stat&  0x1) {
>> +                       buf_pos = buf + i * host->pmecc_sector_size;
>> +
>> +                       pmecc_gen_syndrome(mtd, i);
>> +                       pmecc_substitute(mtd);
>> +                       pmecc_get_sigma(mtd);
>> +
>> +                       err_nbr = pmecc_err_location(mtd);
>> +                       if (err_nbr == -1) {
>> +                               dev_err(host->dev, "PMECC: Too many errors\n");
>> +                               mtd->ecc_stats.failed++;
>> +                               return -EIO;
>> +                       } else {
>> +                               pmecc_correct_data(mtd, buf_pos, 0, err_nbr);
> IHMO, extra_bytes should be (eccbytes / host->pmecc_sector_number)
> pmecc_correct_data(mtd, buf_pos, ecc, eccbytes /
> host->pmecc_sector_number, err_nbr, i);
>
>> +                               mtd->ecc_stats.corrected += err_nbr;
>> +                       }
>> +               }
>> +               pmecc_stat>>= 1;
>> +       }
>> +
>> +       return 0;
>> +}
> I tested the patchset against 3.5-rc4, on a at91sam9g35-ek board.
> the DTS config I used is:
> reg =<0x40000000 0x10000000 0xffffe000 0x600 0xffffe600 0x200
> 0x00100000 0x00100000>;
> nand-bus-width =<8>;
> nand-ecc-mode = "hw";
> nand-on-flash-bbt;
> atmel,nand-addr-offset =<21>;
> atmel,nand-cmd-offset =<22>;
> gpios =<&pioD 5 0&pioD 4 0 0>;
> atmel,has-pmecc;
> atmel,pmecc-cap =<2>;
> atmel,pmecc-sector-size =<512>;
> atmel,pmecc-lookup-table-offset =<0x8000 0x10000>;
>
> (nand on the -ek is 2KiB page size, 128KiB eraseblock)
>
> I tested
> - 1 bit flip on a page
> - 4 bit flips on a page (1 per sector)
> - 5 bit flips on a page (2 on sector 1, and then 1 per sector)
> - 1 bit flip on ECC
> - 2 bit flips on ECC (on the same byte)
> - 3 bit flips =>  I/O error (ok, since pmecc-cap = 2 )
> - 3 bit flips on ECC =>  I/O error (ok, since pmecc-cap = 2 )
> only problem : ECC is not corrected (should be better with the fixes above)
>
> When you have corrected the ECC correction issue, you can add my:
> Tested-by: Richard Genoud<richard.genoud at gmail.com>
>
> Regards.