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

[Richard Genoud]

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.
-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?