[U-Boot,v2,3/5] at91: atmel_nand: Update driver to support Programmable Multibit ECC controller

The Programmable Multibit ECC (PMECC) controller is a programmable binary
BCH(Bose, Chaudhuri and Hocquenghem) encoder and decoder. This controller
can be used to support both SLC and MLC NAND Flash devices. It supports to
generate ECC to correct 2, 4, 8, 12 or 24 bits of error per sector of data.

To use PMECC in this driver, the user needs to set the PMECC correction
capability, the sector size and ROM lookup table offsets in board config file.

This driver is ported from Linux kernel atmel_nand PMECC patch. The main difference
is in this version it uses registers structure access hardware instead of using macros.
It is tested in 9x5 serial boards.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
Changes since v1:
  Change 'ecc' array's type from u32 to u8 in structure pmecc_regs (u32 ecc[11] -> u8 ecc[44]). That will make PMECC write correctly.
  enable 4k-page nand flash pmecc support.
  fix coding style errors and warnings.
  changed lookup table variable name which sounds correct.

 drivers/mtd/nand/atmel_nand.c     |  654 ++++++++++++++++++++++++++++++++++++-
 drivers/mtd/nand/atmel_nand_ecc.h |  111 +++++++
 2 files changed, 763 insertions(+), 2 deletions(-)

Dear Josh Wu,

On 16.08.2012 07:05, Josh Wu wrote:
> The Programmable Multibit ECC (PMECC) controller is a programmable binary
> BCH(Bose, Chaudhuri and Hocquenghem) encoder and decoder. This controller
> can be used to support both SLC and MLC NAND Flash devices. It supports to
> generate ECC to correct 2, 4, 8, 12 or 24 bits of error per sector of data.
> 
> To use PMECC in this driver, the user needs to set the PMECC correction
> capability, the sector size and ROM lookup table offsets in board config file.
> 
> This driver is ported from Linux kernel atmel_nand PMECC patch. The main difference
> is in this version it uses registers structure access hardware instead of using macros.
> It is tested in 9x5 serial boards.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> Changes since v1:
>   Change 'ecc' array's type from u32 to u8 in structure pmecc_regs (u32 ecc[11] -> u8 ecc[44]). That will make PMECC write correctly.
>   enable 4k-page nand flash pmecc support.
>   fix coding style errors and warnings.
>   changed lookup table variable name which sounds correct.
> 
>  drivers/mtd/nand/atmel_nand.c     |  654 ++++++++++++++++++++++++++++++++++++-
>  drivers/mtd/nand/atmel_nand_ecc.h |  111 +++++++
>  2 files changed, 763 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index 9dc003e..784370c 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -5,6 +5,9 @@
>   *
>   * (C) Copyright 2006 ATMEL Rousset, Lacressonniere Nicolas
>   *
> + * Add Programmable Multibit ECC support for various AT91 SoC
> + *     (C) Copyright 2012 ATMEL, Hong Xu
> + *
>   * See file CREDITS for list of people who contributed to this
>   * project.
>   *
> @@ -41,6 +44,648 @@
>  
>  #include "atmel_nand_ecc.h"	/* Hardware ECC registers */
>  
> +static int chip_nr;
> +
> +#ifdef CONFIG_ATMEL_NAND_HW_PMECC
> +
> +struct atmel_nand_host {
> +	struct pmecc_regs __iomem *pmecc;
> +	struct pmecc_errloc_regs __iomem *pmerrloc;
> +	void __iomem		*pmecc_rom_base;
> +
> +	u8		pmecc_corr_cap;
> +	u16		pmecc_sector_size;
> +	u32		pmecc_index_table_offset;
> +
> +	int		pmecc_bytes_per_sector;
> +	int		pmecc_sector_number;
> +	int		pmecc_degree;	/* Degree of remainders */
> +	int		pmecc_cw_len;	/* Length of codeword */
> +
> +	/* lookup table for alpha_to and index_of */
> +	void __iomem	*pmecc_alpha_to;
> +	void __iomem	*pmecc_index_of;
> +
> +	/* data for pmecc computation */
> +	int16_t	pmecc_smu[(CONFIG_PMECC_CAP + 2) * (2 * CONFIG_PMECC_CAP + 1)];
> +	int16_t	pmecc_partial_syn[2 * CONFIG_PMECC_CAP + 1];
> +	int16_t	pmecc_si[2 * CONFIG_PMECC_CAP + 1];
> +	int16_t	pmecc_lmu[CONFIG_PMECC_CAP + 1]; /* polynomal order */
> +	int	pmecc_mu[CONFIG_PMECC_CAP + 1];
> +	int	pmecc_dmu[CONFIG_PMECC_CAP + 1];
> +	int	pmecc_delta[CONFIG_PMECC_CAP + 1];

can you please add some README entry describing these new config parameters?
Namely CONFIG_ATMEL_NAND_HW_PMECC, CONFIG_PMECC_CAP,
CONFIG_PMECC_SECTOR_SIZE (can't this be derived from some already
available NAND information?) and CONFIG_PMECC_INDEX_TABLE_OFFSET.

> +};
> +
> +static struct atmel_nand_host pmecc_host;
> +static struct nand_ecclayout atmel_pmecc_oobinfo;
> +
> +/*
> + * Return number of ecc bytes per sector according to sector size and
> + * correction capability
> + *
> + * Following table shows what at91 PMECC supported:
> + * Correction Capability	Sector_512_bytes	Sector_1024_bytes
> + * =====================	================	=================
> + *                2-bits                 4-bytes                  4-bytes
> + *                4-bits                 7-bytes                  7-bytes
> + *                8-bits                13-bytes                 14-bytes
> + *               12-bits                20-bytes                 21-bytes
> + *               24-bits                39-bytes                 42-bytes
> + */
> +static int pmecc_get_ecc_bytes(int cap, int sector_size)
> +{
> +	int m = 12 + sector_size / 512;
> +	return (m * cap + 7) / 8;
> +}
> +
> +static void pmecc_config_ecc_layout(struct nand_ecclayout *layout,
> +	int oobsize, int ecc_len)
> +{
> +	int i;
> +
> +	layout->eccbytes = ecc_len;
> +
> +	/* ECC will occupy the last ecc_len bytes continuously */
> +	for (i = 0; i < ecc_len; i++)
> +		layout->eccpos[i] = oobsize - ecc_len + i;
> +
> +	layout->oobfree[0].offset = 2;
> +	layout->oobfree[0].length =
> +		oobsize - ecc_len - layout->oobfree[0].offset;
> +}
> +
> +static void __iomem *pmecc_get_alpha_to(struct atmel_nand_host *host)
> +{
> +	int table_size;
> +
> +	table_size = host->pmecc_sector_size == 512 ?
> +		PMECC_INDEX_TABLE_SIZE_512 : PMECC_INDEX_TABLE_SIZE_1024;
> +
> +	/* the ALPHA lookup table is right behind the INDEX lookup table. */
> +	return host->pmecc_rom_base + host->pmecc_index_table_offset +
> +			table_size * sizeof(int16_t);
> +}
> +
> +static void pmecc_gen_syndrome(struct mtd_info *mtd, int sector)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct atmel_nand_host *host = nand_chip->priv;
> +	int i;
> +	uint32_t value;
> +
> +	/* Fill odd syndromes */
> +	for (i = 0; i < host->pmecc_corr_cap; i++) {
> +		value = readl(&host->pmecc->rem_port[sector].rem[i / 2]);
> +		if (i & 1)
> +			value >>= 16;
> +		value &= 0xffff;
> +		host->pmecc_partial_syn[(2 * i) + 1] = (int16_t)value;
> +	}
> +}
> +
> +static void pmecc_substitute(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct atmel_nand_host *host = nand_chip->priv;
> +	int16_t __iomem *alpha_to = host->pmecc_alpha_to;
> +	int16_t __iomem *index_of = host->pmecc_index_of;
> +	int16_t *partial_syn = host->pmecc_partial_syn;
> +	const int cap = host->pmecc_corr_cap;
> +	int16_t *si;
> +	int i, j;
> +
> +	/* si[] is a table that holds the current syndrome value,
> +	 * an element of that table belongs to the field
> +	 */
> +	si = host->pmecc_si;
> +
> +	memset(&si[1], 0, sizeof(int16_t) * (2 * cap - 1));
> +
> +	/* Computation 2t syndromes based on S(x) */
> +	/* Odd syndromes */
> +	for (i = 1; i < 2 * cap; i += 2) {
> +		for (j = 0; j < host->pmecc_degree; j++) {
> +			if (partial_syn[i] & ((unsigned short)0x1 << j))
> +				si[i] = readw(alpha_to + i * j) ^ si[i];
> +		}
> +	}
> +	/* Even syndrome = (Odd syndrome) ** 2 */
> +	for (i = 2, j = 1; j <= cap; i = ++j << 1) {
> +		if (si[j] == 0) {
> +			si[i] = 0;
> +		} else {
> +			int16_t tmp;
> +
> +			tmp = readw(index_of + si[j]);
> +			tmp = (tmp * 2) % host->pmecc_cw_len;
> +			si[i] = readw(alpha_to + tmp);
> +		}
> +	}
> +
> +	return;
> +}
> +
> +static void pmecc_get_sigma(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct atmel_nand_host *host = nand_chip->priv;
> +
> +	int16_t *lmu = host->pmecc_lmu;
> +	int16_t *si = host->pmecc_si;
> +	int *mu = host->pmecc_mu;
> +	int *dmu = host->pmecc_dmu;	/* Discrepancy */
> +	int *delta = host->pmecc_delta; /* Delta order */
> +	int cw_len = host->pmecc_cw_len;
> +	const int16_t cap = host->pmecc_corr_cap;
> +	const int num = 2 * cap + 1;
> +	int16_t __iomem	*index_of = host->pmecc_index_of;
> +	int16_t __iomem	*alpha_to = host->pmecc_alpha_to;
> +	int i, j, k;
> +	uint32_t dmu_0_count, tmp;
> +	int16_t *smu = host->pmecc_smu;
> +
> +	/* index of largest delta */
> +	int ro;
> +	int largest;
> +	int diff;
> +
> +	dmu_0_count = 0;
> +
> +	/* First Row */
> +
> +	/* Mu */
> +	mu[0] = -1;
> +
> +	memset(smu, 0, sizeof(int16_t) * num);
> +	smu[0] = 1;
> +
> +	/* discrepancy set to 1 */
> +	dmu[0] = 1;
> +	/* polynom order set to 0 */
> +	lmu[0] = 0;
> +	delta[0] = (mu[0] * 2 - lmu[0]) >> 1;
---------------------^
isn't mu[0] -1 here
> +
> +	/* Second Row */
> +
> +	/* Mu */
> +	mu[1] = 0;
> +	/* Sigma(x) set to 1 */
> +	memset(&smu[num], 0, sizeof(int16_t) * num);
> +	smu[num] = 1;
> +
> +	/* discrepancy set to S1 */
> +	dmu[1] = si[1];
> +
> +	/* polynom order set to 0 */
> +	lmu[1] = 0;
> +
> +	delta[1] = (mu[1] * 2 - lmu[1]) >> 1;
----------------------^
isn't mu[1] 0 here?

I see, this algorithm is just copied from the processors datasheet.

> +
> +	/* Init the Sigma(x) last row */
> +	memset(&smu[(cap + 1) * num], 0, sizeof(int16_t) * num);

Isn't the whole smu zeroed here at this stage? why not just call
memset(smu, 0, sizeof(int16) * ARRAY_SIZE(smu))?

> +
> +	for (i = 1; i <= cap; i++) {
> +		mu[i + 1] = i << 1;
> +		/* Begin Computing Sigma (Mu+1) and L(mu) */
> +		/* check if discrepancy is set to 0 */
> +		if (dmu[i] == 0) {
> +			dmu_0_count++;
> +
> +			tmp = ((cap - (lmu[i] >> 1) - 1) / 2);
> +			if ((cap - (lmu[i] >> 1) - 1) & 0x1)
> +				tmp += 2;
> +			else
> +				tmp += 1;
> +
> +			if (dmu_0_count == tmp) {
> +				for (j = 0; j <= (lmu[i] >> 1) + 1; j++)
> +					smu[(cap + 1) * num + j] =
> +							smu[i * num + j];
> +
> +				lmu[cap + 1] = lmu[i];
> +				return;
> +			}
> +
> +			/* copy polynom */
> +			for (j = 0; j <= lmu[i] >> 1; j++)
> +				smu[(i + 1) * num + j] = smu[i * num + j];
> +
> +			/* copy previous polynom order to the next */
> +			lmu[i + 1] = lmu[i];

Horrible to read, this was written first by an electrical engineer
(insider joke)? Can you please add a comment at top of pmecc_get_sigma
and add a warning about extremely complicated to read algorithm ;) But
it must be right cause it is written down in the datasheet ...

> +		} else {
> +			ro = 0;
> +			largest = -1;
> +			/* find largest delta with dmu != 0 */
> +			for (j = 0; j < i; j++) {
> +				if ((dmu[j]) && (delta[j] > largest)) {
> +					largest = delta[j];
> +					ro = j;
> +				}
> +			}
> +
> +			/* compute difference */
> +			diff = (mu[i] - mu[ro]);
> +
> +			/* Compute degree of the new smu polynomial */
> +			if ((lmu[i] >> 1) > ((lmu[ro] >> 1) + diff))
> +				lmu[i + 1] = lmu[i];
> +			else
> +				lmu[i + 1] = ((lmu[ro] >> 1) + diff) * 2;
> +
> +			/* Init smu[i+1] with 0 */
> +			for (k = 0; k < num; k++)
> +				smu[(i + 1) * num + k] = 0;
> +
> +			/* Compute smu[i+1] */
> +			for (k = 0; k <= lmu[ro] >> 1; k++) {
> +				int16_t a, b, c;
> +
> +				if (!(smu[ro * num + k] && dmu[i]))
> +					continue;
> +				a = readw(index_of + dmu[i]);
> +				b = readw(index_of + dmu[ro]);
> +				c = readw(index_of + smu[ro * num + k]);
> +				tmp = a + (cw_len - b) + c;
> +				a = readw(alpha_to + tmp % cw_len);
> +				smu[(i + 1) * num + (k + diff)] = a;
> +			}
> +
> +			for (k = 0; k <= lmu[i] >> 1; k++)
> +				smu[(i + 1) * num + k] ^= smu[i * num + k];
> +		}
> +
> +		/* End Computing Sigma (Mu+1) and L(mu) */
> +		/* In either case compute delta */
> +		delta[i + 1] = (mu[i + 1] * 2 - lmu[i + 1]) >> 1;
> +
> +		/* Do not compute discrepancy for the last iteration */
> +		if (i >= cap)
> +			continue;
> +
> +		for (k = 0; k <= (lmu[i + 1] >> 1); k++) {
> +			tmp = 2 * (i - 1);
> +			if (k == 0) {
> +				dmu[i + 1] = si[tmp + 3];
> +			} else if (smu[(i + 1) * num + k] && si[tmp + 3 - k]) {
> +				int16_t a, b, c;
> +				a = readw(index_of +
> +						smu[(i + 1) * num + k]);
> +				b = si[2 * (i - 1) + 3 - k];
> +				c = readw(index_of + b);
> +				tmp = a + c;
> +				tmp %= cw_len;
> +				dmu[i + 1] = readw(alpha_to + tmp) ^
> +					dmu[i + 1];
> +			}
> +		}
> +	}
> +
> +	return;
> +}
> +
> +static int pmecc_err_location(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct atmel_nand_host *host = nand_chip->priv;
> +	const int cap = host->pmecc_corr_cap;
> +	const int num = 2 * cap + 1;
> +	int sector_size = host->pmecc_sector_size;
> +	int err_nbr = 0;	/* number of error */
> +	int roots_nbr;		/* number of roots */
> +	int i;
> +	uint32_t val;
> +	int16_t *smu = host->pmecc_smu;
> +
> +	writel(PMERRLOC_DISABLE, &host->pmerrloc->eldis);
> +
> +	for (i = 0; i <= host->pmecc_lmu[cap + 1] >> 1; i++) {
> +		writel(smu[(cap + 1) * num + i], &host->pmerrloc->sigma[i]);
> +		err_nbr++;
> +	}
> +
> +	val = PMERRLOC_ELCFG_NUM_ERRORS(err_nbr - 1);
> +	if (sector_size == 1024)
> +		val |= PMERRLOC_ELCFG_SECTOR_1024;
> +
> +	writel(val, &host->pmerrloc->elcfg);
> +	writel(sector_size * 8 + host->pmecc_degree * cap,
> +			&host->pmerrloc->elen);
> +
> +	while (!(readl(&host->pmerrloc->elisr) & PMERRLOC_CALC_DONE))
> +		udelay(10);

Can you please add some timeout here. Maybe a WATCHDOG_RESET is also
required?

> +
> +	roots_nbr = (readl(&host->pmerrloc->elisr) & PMERRLOC_ERR_NUM_MASK)
> +			>> 8;
> +	/* Number of roots == degree of smu hence <= cap */
> +	if (roots_nbr == host->pmecc_lmu[cap + 1] >> 1)
> +		return err_nbr - 1;
> +
> +	/* Number of roots does not match the degree of smu
> +	 * unable to correct error */
> +	return -1;
> +}
> +
> +static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
> +		int sector_num, 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, pos;
> +	uint32_t tmp;
> +	uint8_t err_byte;
> +
> +	sector_size = host->pmecc_sector_size;
> +
> +	while (err_nbr) {
> +		tmp = readl(&host->pmerrloc->el[i]) - 1;
> +		byte_pos = tmp / 8;
> +		bit_pos  = tmp % 8;
> +
> +		if (byte_pos >= (sector_size + extra_bytes))
> +			BUG();	/* should never happen */
> +
> +		if (byte_pos < sector_size) {
> +			err_byte = *(buf + byte_pos);
> +			*(buf + byte_pos) ^= (1 << bit_pos);
> +
> +			pos = sector_num * host->pmecc_sector_size + byte_pos;
> +			printk(KERN_INFO "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
> +				pos, bit_pos, err_byte, *(buf + byte_pos));
> +		} else {
> +			/* Bit flip in OOB area */
> +			tmp = sector_num * host->pmecc_bytes_per_sector
> +					+ (byte_pos - sector_size);
> +			err_byte = ecc[tmp];
> +			ecc[tmp] ^= (1 << bit_pos);
> +
> +			pos = tmp + nand_chip->ecc.layout->eccpos[0];
> +			printk(KERN_INFO "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
> +				pos, bit_pos, err_byte, ecc[tmp]);
> +		}
> +
> +		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) {
> +				printk(KERN_ERR "PMECC: Too many errors\n");
> +				mtd->ecc_stats.failed++;
> +				return -EIO;
> +			} else {
> +				pmecc_correct_data(mtd, buf_pos, ecc, i,
> +					host->pmecc_bytes_per_sector, err_nbr);
> +				mtd->ecc_stats.corrected += err_nbr;
> +			}
> +		}
> +		pmecc_stat >>= 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int atmel_nand_pmecc_read_page(struct mtd_info *mtd,
> +	struct nand_chip *chip, uint8_t *buf, int page)
> +{
> +	struct atmel_nand_host *host = chip->priv;
> +	int eccsize = chip->ecc.size;
> +	uint8_t *oob = chip->oob_poi;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	uint32_t stat;
> +
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_RST);
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DISABLE);
> +	pmecc_writel(host->pmecc, cfg, ((pmecc_readl(host->pmecc, cfg))
> +		& ~PMECC_CFG_WRITE_OP) | PMECC_CFG_AUTO_ENABLE);
> +
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_ENABLE);
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DATA);
> +
> +	chip->read_buf(mtd, buf, eccsize);
> +	chip->read_buf(mtd, oob, mtd->oobsize);
> +
> +	while ((pmecc_readl(host->pmecc, sr) & PMECC_SR_BUSY))
> +		udelay(1);
> +
> +	stat = pmecc_readl(host->pmecc, isr);
> +	if (stat != 0)
> +		if (pmecc_correction(mtd, stat, buf, &oob[eccpos[0]]) != 0)
> +			return -EIO;
> +
> +	return 0;
> +}
> +
> +static void atmel_nand_pmecc_write_page(struct mtd_info *mtd,
> +		struct nand_chip *chip, const uint8_t *buf)
> +{
> +	struct atmel_nand_host *host = chip->priv;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	int i, j;
> +
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_RST);
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DISABLE);
> +
> +	pmecc_writel(host->pmecc, cfg, (pmecc_readl(host->pmecc, cfg) |
> +		PMECC_CFG_WRITE_OP) & ~PMECC_CFG_AUTO_ENABLE);
> +
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_ENABLE);
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DATA);
> +
> +	chip->write_buf(mtd, (u8 *)buf, mtd->writesize);
> +
> +	while ((pmecc_readl(host->pmecc, sr) & PMECC_SR_BUSY))
> +		udelay(1);
> +
> +	for (i = 0; i < host->pmecc_sector_number; i++) {
> +		for (j = 0; j < host->pmecc_bytes_per_sector; j++) {
> +			int pos;
> +
> +			pos = i * host->pmecc_bytes_per_sector + j;
> +			chip->oob_poi[eccpos[pos]] =
> +				readb(&host->pmecc->ecc_port[i].ecc[j]);
> +		}
> +	}
> +	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +}
> +
> +static void atmel_pmecc_core_init(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	struct atmel_nand_host *host = nand_chip->priv;
> +	uint32_t val = 0;
> +	struct nand_ecclayout *ecc_layout;
> +
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_RST);
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DISABLE);
> +
> +	switch (host->pmecc_corr_cap) {
> +	case 2:
> +		val = PMECC_CFG_BCH_ERR2;
> +		break;
> +	case 4:
> +		val = PMECC_CFG_BCH_ERR4;
> +		break;
> +	case 8:
> +		val = PMECC_CFG_BCH_ERR8;
> +		break;
> +	case 12:
> +		val = PMECC_CFG_BCH_ERR12;
> +		break;
> +	case 24:
> +		val = PMECC_CFG_BCH_ERR24;
> +		break;
> +	}
> +
> +	if (host->pmecc_sector_size == 512)
> +		val |= PMECC_CFG_SECTOR512;
> +	else if (host->pmecc_sector_size == 1024)
> +		val |= PMECC_CFG_SECTOR1024;
> +
> +	switch (host->pmecc_sector_number) {
> +	case 1:
> +		val |= PMECC_CFG_PAGE_1SECTOR;
> +		break;
> +	case 2:
> +		val |= PMECC_CFG_PAGE_2SECTORS;
> +		break;
> +	case 4:
> +		val |= PMECC_CFG_PAGE_4SECTORS;
> +		break;
> +	case 8:
> +		val |= PMECC_CFG_PAGE_8SECTORS;
> +		break;
> +	}
> +
> +	val |= (PMECC_CFG_READ_OP | PMECC_CFG_SPARE_DISABLE
> +		| PMECC_CFG_AUTO_DISABLE);
> +	pmecc_writel(host->pmecc, cfg, val);
> +
> +	ecc_layout = nand_chip->ecc.layout;
> +	pmecc_writel(host->pmecc, sarea, mtd->oobsize - 1);
> +	pmecc_writel(host->pmecc, saddr, ecc_layout->eccpos[0]);
> +	pmecc_writel(host->pmecc, eaddr,
> +			ecc_layout->eccpos[ecc_layout->eccbytes - 1]);
> +	/* See datasheet about PMECC Clock Control Register */
> +	pmecc_writel(host->pmecc, clk, PMECC_CLK_133MHZ);
> +	pmecc_writel(host->pmecc, idr, 0xff);
> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_ENABLE);
> +}
> +
> +static int atmel_pmecc_nand_init_params(struct nand_chip *nand)
> +{
> +	struct mtd_info *mtd;
> +	struct atmel_nand_host *host;
> +	int cap, sector_size;
> +
> +	mtd = &nand_info[chip_nr++];

NAK

> +	mtd->priv = nand;
> +	host = nand->priv = &pmecc_host;
> +
> +	/* Detect NAND chips */
> +	if (nand_scan_ident(mtd, 1, NULL)) {

please change that, read later on.

> +		printk(KERN_WARNING "NAND Flash not found !\n");
> +		return -ENXIO;
> +	}
> +
> +	nand->ecc.mode = NAND_ECC_HW;
> +	nand->ecc.calculate = NULL;
> +	nand->ecc.correct = NULL;
> +	nand->ecc.hwctl = NULL;
> +
> +	cap = host->pmecc_corr_cap = CONFIG_PMECC_CAP;
> +	sector_size = host->pmecc_sector_size = CONFIG_PMECC_SECTOR_SIZE;

Can't we use some information from the nand chip here? Do we need to
hard code this paramater?

> +	host->pmecc_index_table_offset = CONFIG_PMECC_INDEX_TABLE_OFFSET;
> +
> +	printk(KERN_INFO "Initialize PMECC params, cap: %d, sector: %d\n",
> +		 cap, sector_size);
> +
> +	host->pmecc = (struct pmecc_regs __iomem *) ATMEL_BASE_PMECC;
> +	host->pmerrloc = (struct pmecc_errloc_regs __iomem *)
> +			ATMEL_BASE_PMERRLOC;
> +	host->pmecc_rom_base = (void __iomem *) ATMEL_BASE_ROM;
> +
> +	/* ECC is calculated for the whole page (1 step) */
> +	nand->ecc.size = mtd->writesize;
> +
> +	/* set ECC page size and oob layout */
> +	switch (mtd->writesize) {
> +	case 2048:
> +	case 4096:
> +		host->pmecc_degree = PMECC_GF_DIMENSION_13;
> +		host->pmecc_cw_len = (1 << host->pmecc_degree) - 1;
> +		host->pmecc_sector_number = mtd->writesize / sector_size;
> +		host->pmecc_bytes_per_sector = pmecc_get_ecc_bytes(
> +			cap, sector_size);
> +		host->pmecc_alpha_to = pmecc_get_alpha_to(host);
> +		host->pmecc_index_of = host->pmecc_rom_base +
> +			host->pmecc_index_table_offset;
> +
> +		nand->ecc.steps = 1;
> +		nand->ecc.bytes = host->pmecc_bytes_per_sector *
> +				       host->pmecc_sector_number;
> +		if (nand->ecc.bytes > mtd->oobsize - 2) {
> +			printk(KERN_ERR "No room for ECC bytes\n");
> +			return -EINVAL;
> +		}
> +		pmecc_config_ecc_layout(&atmel_pmecc_oobinfo,
> +					mtd->oobsize,
> +					nand->ecc.bytes);
> +		nand->ecc.layout = &atmel_pmecc_oobinfo;
> +		break;
> +	case 512:
> +	case 1024:
> +		/* TODO */
> +		printk(KERN_ERR "Unsupported page size for PMECC, use Software ECC\n");
> +	default:
> +		/* page size not handled by HW ECC */
> +		/* switching back to soft ECC */
> +		nand->ecc.mode = NAND_ECC_SOFT;
> +		nand->ecc.read_page = NULL;
> +		nand->ecc.postpad = 0;
> +		nand->ecc.prepad = 0;
> +		nand->ecc.bytes = 0;
> +		return 0;
> +	}
> +
> +	nand->ecc.read_page = atmel_nand_pmecc_read_page;
> +	nand->ecc.write_page = atmel_nand_pmecc_write_page;
> +
> +	atmel_pmecc_core_init(mtd);
> +
> +	return 0;
> +}
> +
> +#else
> +
>  /* oob layout for large page size
>   * bad block info is on bytes 0 and 1
>   * the bytes have to be consecutives to avoid
> @@ -234,7 +879,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode)
>  
>  int atmel_hw_nand_init_param(struct nand_chip *nand)
>  {
> -	static int chip_nr = 0;
>  	struct mtd_info *mtd;
>  
>  	nand->ecc.mode = NAND_ECC_HW;
> @@ -293,7 +937,9 @@ int atmel_hw_nand_init_param(struct nand_chip *nand)
>  	return 0;
>  }
>  
> -#endif
> +#endif /* CONFIG_ATMEL_NAND_HW_PMECC */
> +
> +#endif /* CONFIG_ATMEL_NAND_HWECC */
>  
>  static void at91_nand_hwcontrol(struct mtd_info *mtd,
>  					 int cmd, unsigned int ctrl)
> @@ -343,8 +989,12 @@ int board_nand_init(struct nand_chip *nand)
>  	nand->chip_delay = 20;
>  
>  #ifdef CONFIG_ATMEL_NAND_HWECC
> +#ifdef CONFIG_ATMEL_NAND_HW_PMECC
> +	res = atmel_pmecc_nand_init_params(nand);
> +#else
>  	res = atmel_hw_nand_init_param(nand);
>  #endif
> +#endif
>  
>  	return res;
>  }
> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
> index 1ee7f99..8f06b14 100644
> --- a/drivers/mtd/nand/atmel_nand_ecc.h
> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
> @@ -33,4 +33,115 @@
>  #define ATMEL_ECC_NPR		0x10			/* NParity register */
>  #define		ATMEL_ECC_NPARITY	(0xffff << 0)		/* NParity */
>  
> +/* Register access macros for PMECC */
> +#define pmecc_readl(addr, reg) \
> +	readl(&addr->reg)
> +
> +#define pmecc_writel(addr, reg, value) \
> +	writel((value), &addr->reg)
> +
> +/* PMECC Register Definitions */
> +#define PMECC_MAX_SECTOR_NUM			8
> +struct pmecc_regs {
> +	u32 cfg;		/* 0x00 PMECC Configuration Register */
> +	u32 sarea;		/* 0x04 PMECC Spare Area Size Register */
> +	u32 saddr;		/* 0x08 PMECC Start Address Register */
> +	u32 eaddr;		/* 0x0C PMECC End Address Register */
> +	u32 clk;		/* 0x10 PMECC Clock Control Register */
> +	u32 ctrl;		/* 0x14 PMECC Control Register */
> +	u32 sr;			/* 0x18 PMECC Status Register */
> +	u32 ier;		/* 0x1C PMECC Interrupt Enable Register */
> +	u32 idr;		/* 0x20 PMECC Interrupt Disable Register */
> +	u32 imr;		/* 0x24 PMECC Interrupt Mask Register */
> +	u32 isr;		/* 0x28 PMECC Interrupt Status Register */
> +	u32 reserved0[5];	/* 0x2C-0x3C Reserved */
> +
> +	/* 0x40 + sector_num * (0x40), Redundancy Registers */
> +	struct {
> +		u8 ecc[44];	/* PMECC Generated Redundancy Byte Per Sector */
> +		u32 reserved1[5];
> +	} ecc_port[PMECC_MAX_SECTOR_NUM];
> +
> +	/* 0x240 + sector_num * (0x40) Remainder Registers */
> +	struct {
> +		u32 rem[12];
> +		u32 reserved2[4];
> +	} rem_port[PMECC_MAX_SECTOR_NUM];
> +	u32 reserved3[16];	/* 0x440-0x47C Reserved */
> +};
> +
> +/* For PMECC Configuration Register */
> +#define		PMECC_CFG_BCH_ERR2		(0 << 0)
> +#define		PMECC_CFG_BCH_ERR4		(1 << 0)
> +#define		PMECC_CFG_BCH_ERR8		(2 << 0)
> +#define		PMECC_CFG_BCH_ERR12		(3 << 0)
> +#define		PMECC_CFG_BCH_ERR24		(4 << 0)
> +
> +#define		PMECC_CFG_SECTOR512		(0 << 4)
> +#define		PMECC_CFG_SECTOR1024		(1 << 4)
> +
> +#define		PMECC_CFG_PAGE_1SECTOR		(0 << 8)
> +#define		PMECC_CFG_PAGE_2SECTORS		(1 << 8)
> +#define		PMECC_CFG_PAGE_4SECTORS		(2 << 8)
> +#define		PMECC_CFG_PAGE_8SECTORS		(3 << 8)
> +
> +#define		PMECC_CFG_READ_OP		(0 << 12)
> +#define		PMECC_CFG_WRITE_OP		(1 << 12)
> +
> +#define		PMECC_CFG_SPARE_ENABLE		(1 << 16)
> +#define		PMECC_CFG_SPARE_DISABLE		(0 << 16)
> +
> +#define		PMECC_CFG_AUTO_ENABLE		(1 << 20)
> +#define		PMECC_CFG_AUTO_DISABLE		(0 << 20)
> +
> +/* For PMECC Clock Control Register */
> +#define		PMECC_CLK_133MHZ		(2 << 0)
> +
> +/* For PMECC Control Register */
> +#define		PMECC_CTRL_RST			(1 << 0)
> +#define		PMECC_CTRL_DATA			(1 << 1)
> +#define		PMECC_CTRL_USER			(1 << 2)
> +#define		PMECC_CTRL_ENABLE		(1 << 4)
> +#define		PMECC_CTRL_DISABLE		(1 << 5)
> +
> +/* For PMECC Status Register */
> +#define		PMECC_SR_BUSY			(1 << 0)
> +#define		PMECC_SR_ENABLE			(1 << 4)
> +
> +/* PMERRLOC Register Definitions */
> +struct pmecc_errloc_regs {
> +	u32 elcfg;	/* 0x00 Error Location Configuration Register */
> +	u32 elprim;	/* 0x04 Error Location Primitive Register */
> +	u32 elen;	/* 0x08 Error Location Enable Register */
> +	u32 eldis;	/* 0x0C Error Location Disable Register */
> +	u32 elsr;	/* 0x10 Error Location Status Register */
> +	u32 elier;	/* 0x14 Error Location Interrupt Enable Register */
> +	u32 elidr;	/* 0x08 Error Location Interrupt Disable Register */
> +	u32 elimr;	/* 0x0C Error Location Interrupt Mask Register */
> +	u32 elisr;	/* 0x20 Error Location Interrupt Status Register */
> +	u32 reserved0;	/* 0x24 Reserved */
> +	u32 sigma[25];	/* 0x28-0x88 Error Location Sigma Registers */
> +	u32 el[24];	/* 0x8C-0xE8 Error Location Registers */
> +	u32 reserved1[5];	/* 0xEC-0xFC Reserved */
> +};
> +
> +/* For Error Location Configuration Register */
> +#define		PMERRLOC_ELCFG_SECTOR_512	(0 << 0)
> +#define		PMERRLOC_ELCFG_SECTOR_1024	(1 << 0)
> +#define		PMERRLOC_ELCFG_NUM_ERRORS(n)	((n) << 16)
> +
> +/* For Error Location Disable Register */
> +#define		PMERRLOC_DISABLE		(1 << 0)
> +
> +/* For Error Location Interrupt Status Register */
> +#define		PMERRLOC_ERR_NUM_MASK		(0x1f << 8)
> +#define		PMERRLOC_CALC_DONE		(1 << 0)
> +
> +/* Galois field dimension */
> +#define PMECC_GF_DIMENSION_13			13
> +#define PMECC_GF_DIMENSION_14			14
> +
> +#define PMECC_INDEX_TABLE_SIZE_512		0x2000
> +#define PMECC_INDEX_TABLE_SIZE_1024		0x4000
> +
>  #endif
> 

Not really nice code but it seems the kernel guys accepted it the same
way. So I will _not_ NAK it. But please add at least a README entry
describing the new config parameters, think about the sector size (ore
explain me why you need another config) and please change the driver
initialization to the SELF_INIT as mentioned in another mail. You use
again the nand_scan_indent() inside your xx_nand_init_params().
Please rewrite the board_nand_init() as shown in doc/README.nand, maybe
copy the stub from mtd/nand.c's nand_init() and nand_init_chip(). Call
in your own atmel_nand_init_chip() nand_scan_ident() and after that
setup the parameters for eighter ECC or PMECC. Always hand over the
struct nand_chip and do not work with the nand_chip array in the
xx_init_param() functions.

Best regards

Andreas Bießmann

On 08/16/2012 12:05 AM, Josh Wu wrote:
> The Programmable Multibit ECC (PMECC) controller is a programmable binary
> BCH(Bose, Chaudhuri and Hocquenghem) encoder and decoder. This controller
> can be used to support both SLC and MLC NAND Flash devices. It supports to
> generate ECC to correct 2, 4, 8, 12 or 24 bits of error per sector of data.
> 
> To use PMECC in this driver, the user needs to set the PMECC correction
> capability, the sector size and ROM lookup table offsets in board config file.
> 
> This driver is ported from Linux kernel atmel_nand PMECC patch. The main difference
> is in this version it uses registers structure access hardware instead of using macros.

Sigh, I wish U-Boot's code style policies weren't so rigid as to force
such a difference relative to the Linux driver.

> +	/* Computation 2t syndromes based on S(x) */
> +	/* Odd syndromes */
> +	for (i = 1; i < 2 * cap; i += 2) {
> +		for (j = 0; j < host->pmecc_degree; j++) {
> +			if (partial_syn[i] & ((unsigned short)0x1 << j))
> +				si[i] = readw(alpha_to + i * j) ^ si[i];
> +		}

Will that (unsigned short) make any difference?  Won't it just get
promoted back to int for the computation?

> +		i++;
> +		err_nbr--;
> +	}
> +
> +	return;
> +}

If it came over from Linux this way, I'm not asking you to change it
just here, but the return; at the end is superfluous.

> +	while ((pmecc_readl(host->pmecc, sr) & PMECC_SR_BUSY))
> +		udelay(1);

Timeout?

-Scott

Hi, Andreas

On 8/17/2012 5:24 PM, Andreas Bießmann wrote:
> Dear Josh Wu,
>
> On 16.08.2012 07:05, Josh Wu wrote:
>> The Programmable Multibit ECC (PMECC) controller is a programmable binary
>> BCH(Bose, Chaudhuri and Hocquenghem) encoder and decoder. This controller
>> can be used to support both SLC and MLC NAND Flash devices. It supports to
>> generate ECC to correct 2, 4, 8, 12 or 24 bits of error per sector of data.
>>
>> To use PMECC in this driver, the user needs to set the PMECC correction
>> capability, the sector size and ROM lookup table offsets in board config file.
>>
>> This driver is ported from Linux kernel atmel_nand PMECC patch. The main difference
>> is in this version it uses registers structure access hardware instead of using macros.
>> It is tested in 9x5 serial boards.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> Changes since v1:
>>    Change 'ecc' array's type from u32 to u8 in structure pmecc_regs (u32 ecc[11] -> u8 ecc[44]). That will make PMECC write correctly.
>>    enable 4k-page nand flash pmecc support.
>>    fix coding style errors and warnings.
>>    changed lookup table variable name which sounds correct.
>>
>>   drivers/mtd/nand/atmel_nand.c     |  654 ++++++++++++++++++++++++++++++++++++-
>>   drivers/mtd/nand/atmel_nand_ecc.h |  111 +++++++
>>   2 files changed, 763 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index 9dc003e..784370c 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -5,6 +5,9 @@
>>    *
>>    * (C) Copyright 2006 ATMEL Rousset, Lacressonniere Nicolas
>>    *
>> + * Add Programmable Multibit ECC support for various AT91 SoC
>> + *     (C) Copyright 2012 ATMEL, Hong Xu
>> + *
>>    * See file CREDITS for list of people who contributed to this
>>    * project.
>>    *
>> @@ -41,6 +44,648 @@
>>   
>>   #include "atmel_nand_ecc.h"	/* Hardware ECC registers */
>>   
>> +static int chip_nr;
>> +
>> +#ifdef CONFIG_ATMEL_NAND_HW_PMECC
>> +
>> +struct atmel_nand_host {
>> +	struct pmecc_regs __iomem *pmecc;
>> +	struct pmecc_errloc_regs __iomem *pmerrloc;
>> +	void __iomem		*pmecc_rom_base;
>> +
>> +	u8		pmecc_corr_cap;
>> +	u16		pmecc_sector_size;
>> +	u32		pmecc_index_table_offset;
>> +
>> +	int		pmecc_bytes_per_sector;
>> +	int		pmecc_sector_number;
>> +	int		pmecc_degree;	/* Degree of remainders */
>> +	int		pmecc_cw_len;	/* Length of codeword */
>> +
>> +	/* lookup table for alpha_to and index_of */
>> +	void __iomem	*pmecc_alpha_to;
>> +	void __iomem	*pmecc_index_of;
>> +
>> +	/* data for pmecc computation */
>> +	int16_t	pmecc_smu[(CONFIG_PMECC_CAP + 2) * (2 * CONFIG_PMECC_CAP + 1)];
>> +	int16_t	pmecc_partial_syn[2 * CONFIG_PMECC_CAP + 1];
>> +	int16_t	pmecc_si[2 * CONFIG_PMECC_CAP + 1];
>> +	int16_t	pmecc_lmu[CONFIG_PMECC_CAP + 1]; /* polynomal order */
>> +	int	pmecc_mu[CONFIG_PMECC_CAP + 1];
>> +	int	pmecc_dmu[CONFIG_PMECC_CAP + 1];
>> +	int	pmecc_delta[CONFIG_PMECC_CAP + 1];
> can you please add some README entry describing these new config parameters?
> Namely CONFIG_ATMEL_NAND_HW_PMECC, CONFIG_PMECC_CAP,
> CONFIG_PMECC_SECTOR_SIZE (can't this be derived from some already
> available NAND information?) and CONFIG_PMECC_INDEX_TABLE_OFFSET.

OK, I will add a README file to explain all the parameters.
this CONFIG_PMECC_SECTOR_SIZE means how many bytes to generate out PMECC 
code. It only can be 512 and 1024.
So for a nand chip whose page size is 2048, if CONFIG_PMECC_SECTOR_SIZE 
is set as 512, then PMECC will generate PMECC code for each 512 bytes.

I think it cannot be derived from nand information.

>
>> +};
>> +
>> +static struct atmel_nand_host pmecc_host;
>> +static struct nand_ecclayout atmel_pmecc_oobinfo;
>> +
>> +/*
>> + * Return number of ecc bytes per sector according to sector size and
>> + * correction capability
>> + *
>> + * Following table shows what at91 PMECC supported:
>> + * Correction Capability	Sector_512_bytes	Sector_1024_bytes
>> + * =====================	================	=================
>> + *                2-bits                 4-bytes                  4-bytes
>> + *                4-bits                 7-bytes                  7-bytes
>> + *                8-bits                13-bytes                 14-bytes
>> + *               12-bits                20-bytes                 21-bytes
>> + *               24-bits                39-bytes                 42-bytes
>> + */
>> +static int pmecc_get_ecc_bytes(int cap, int sector_size)
>> +{
>> +	int m = 12 + sector_size / 512;
>> +	return (m * cap + 7) / 8;
>> +}
>> +
>> +static void pmecc_config_ecc_layout(struct nand_ecclayout *layout,
>> +	int oobsize, int ecc_len)
>> +{
>> +	int i;
>> +
>> +	layout->eccbytes = ecc_len;
>> +
>> +	/* ECC will occupy the last ecc_len bytes continuously */
>> +	for (i = 0; i < ecc_len; i++)
>> +		layout->eccpos[i] = oobsize - ecc_len + i;
>> +
>> +	layout->oobfree[0].offset = 2;
>> +	layout->oobfree[0].length =
>> +		oobsize - ecc_len - layout->oobfree[0].offset;
>> +}
>> +
>> +static void __iomem *pmecc_get_alpha_to(struct atmel_nand_host *host)
>> +{
>> +	int table_size;
>> +
>> +	table_size = host->pmecc_sector_size == 512 ?
>> +		PMECC_INDEX_TABLE_SIZE_512 : PMECC_INDEX_TABLE_SIZE_1024;
>> +
>> +	/* the ALPHA lookup table is right behind the INDEX lookup table. */
>> +	return host->pmecc_rom_base + host->pmecc_index_table_offset +
>> +			table_size * sizeof(int16_t);
>> +}
>> +
>> +static void pmecc_gen_syndrome(struct mtd_info *mtd, int sector)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct atmel_nand_host *host = nand_chip->priv;
>> +	int i;
>> +	uint32_t value;
>> +
>> +	/* Fill odd syndromes */
>> +	for (i = 0; i < host->pmecc_corr_cap; i++) {
>> +		value = readl(&host->pmecc->rem_port[sector].rem[i / 2]);
>> +		if (i & 1)
>> +			value >>= 16;
>> +		value &= 0xffff;
>> +		host->pmecc_partial_syn[(2 * i) + 1] = (int16_t)value;
>> +	}
>> +}
>> +
>> +static void pmecc_substitute(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct atmel_nand_host *host = nand_chip->priv;
>> +	int16_t __iomem *alpha_to = host->pmecc_alpha_to;
>> +	int16_t __iomem *index_of = host->pmecc_index_of;
>> +	int16_t *partial_syn = host->pmecc_partial_syn;
>> +	const int cap = host->pmecc_corr_cap;
>> +	int16_t *si;
>> +	int i, j;
>> +
>> +	/* si[] is a table that holds the current syndrome value,
>> +	 * an element of that table belongs to the field
>> +	 */
>> +	si = host->pmecc_si;
>> +
>> +	memset(&si[1], 0, sizeof(int16_t) * (2 * cap - 1));
>> +
>> +	/* Computation 2t syndromes based on S(x) */
>> +	/* Odd syndromes */
>> +	for (i = 1; i < 2 * cap; i += 2) {
>> +		for (j = 0; j < host->pmecc_degree; j++) {
>> +			if (partial_syn[i] & ((unsigned short)0x1 << j))
>> +				si[i] = readw(alpha_to + i * j) ^ si[i];
>> +		}
>> +	}
>> +	/* Even syndrome = (Odd syndrome) ** 2 */
>> +	for (i = 2, j = 1; j <= cap; i = ++j << 1) {
>> +		if (si[j] == 0) {
>> +			si[i] = 0;
>> +		} else {
>> +			int16_t tmp;
>> +
>> +			tmp = readw(index_of + si[j]);
>> +			tmp = (tmp * 2) % host->pmecc_cw_len;
>> +			si[i] = readw(alpha_to + tmp);
>> +		}
>> +	}
>> +
>> +	return;
>> +}
>> +
>> +static void pmecc_get_sigma(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct atmel_nand_host *host = nand_chip->priv;
>> +
>> +	int16_t *lmu = host->pmecc_lmu;
>> +	int16_t *si = host->pmecc_si;
>> +	int *mu = host->pmecc_mu;
>> +	int *dmu = host->pmecc_dmu;	/* Discrepancy */
>> +	int *delta = host->pmecc_delta; /* Delta order */
>> +	int cw_len = host->pmecc_cw_len;
>> +	const int16_t cap = host->pmecc_corr_cap;
>> +	const int num = 2 * cap + 1;
>> +	int16_t __iomem	*index_of = host->pmecc_index_of;
>> +	int16_t __iomem	*alpha_to = host->pmecc_alpha_to;
>> +	int i, j, k;
>> +	uint32_t dmu_0_count, tmp;
>> +	int16_t *smu = host->pmecc_smu;
>> +
>> +	/* index of largest delta */
>> +	int ro;
>> +	int largest;
>> +	int diff;
>> +
>> +	dmu_0_count = 0;
>> +
>> +	/* First Row */
>> +
>> +	/* Mu */
>> +	mu[0] = -1;
>> +
>> +	memset(smu, 0, sizeof(int16_t) * num);
>> +	smu[0] = 1;
>> +
>> +	/* discrepancy set to 1 */
>> +	dmu[0] = 1;
>> +	/* polynom order set to 0 */
>> +	lmu[0] = 0;
>> +	delta[0] = (mu[0] * 2 - lmu[0]) >> 1;
> ---------------------^
> isn't mu[0] -1 here

yes, sorry, this algorithm is from datasheet. Maybe I can add comment here.

>> +
>> +	/* Second Row */
>> +
>> +	/* Mu */
>> +	mu[1] = 0;
>> +	/* Sigma(x) set to 1 */
>> +	memset(&smu[num], 0, sizeof(int16_t) * num);
>> +	smu[num] = 1;
>> +
>> +	/* discrepancy set to S1 */
>> +	dmu[1] = si[1];
>> +
>> +	/* polynom order set to 0 */
>> +	lmu[1] = 0;
>> +
>> +	delta[1] = (mu[1] * 2 - lmu[1]) >> 1;
> ----------------------^
> isn't mu[1] 0 here?
>
> I see, this algorithm is just copied from the processors datasheet.

actually, I am not similar with this algorithm.

>
>> +
>> +	/* Init the Sigma(x) last row */
>> +	memset(&smu[(cap + 1) * num], 0, sizeof(int16_t) * num);
> Isn't the whole smu zeroed here at this stage? why not just call
> memset(smu, 0, sizeof(int16) * ARRAY_SIZE(smu))?

I will confirm it and above with someone who is familiar with this 
algorithm.

>
>> +
>> +	for (i = 1; i <= cap; i++) {
>> +		mu[i + 1] = i << 1;
>> +		/* Begin Computing Sigma (Mu+1) and L(mu) */
>> +		/* check if discrepancy is set to 0 */
>> +		if (dmu[i] == 0) {
>> +			dmu_0_count++;
>> +
>> +			tmp = ((cap - (lmu[i] >> 1) - 1) / 2);
>> +			if ((cap - (lmu[i] >> 1) - 1) & 0x1)
>> +				tmp += 2;
>> +			else
>> +				tmp += 1;
>> +
>> +			if (dmu_0_count == tmp) {
>> +				for (j = 0; j <= (lmu[i] >> 1) + 1; j++)
>> +					smu[(cap + 1) * num + j] =
>> +							smu[i * num + j];
>> +
>> +				lmu[cap + 1] = lmu[i];
>> +				return;
>> +			}
>> +
>> +			/* copy polynom */
>> +			for (j = 0; j <= lmu[i] >> 1; j++)
>> +				smu[(i + 1) * num + j] = smu[i * num + j];
>> +
>> +			/* copy previous polynom order to the next */
>> +			lmu[i + 1] = lmu[i];
> Horrible to read, this was written first by an electrical engineer
> (insider joke)? Can you please add a comment at top of pmecc_get_sigma
> and add a warning about extremely complicated to read algorithm ;) But
> it must be right cause it is written down in the datasheet ...

ok, i will add more comments for the function. seems it is algorithm 
related. it is indeed to read.

>
>> +		} else {
>> +			ro = 0;
>> +			largest = -1;
>> +			/* find largest delta with dmu != 0 */
>> +			for (j = 0; j < i; j++) {
>> +				if ((dmu[j]) && (delta[j] > largest)) {
>> +					largest = delta[j];
>> +					ro = j;
>> +				}
>> +			}
>> +
>> +			/* compute difference */
>> +			diff = (mu[i] - mu[ro]);
>> +
>> +			/* Compute degree of the new smu polynomial */
>> +			if ((lmu[i] >> 1) > ((lmu[ro] >> 1) + diff))
>> +				lmu[i + 1] = lmu[i];
>> +			else
>> +				lmu[i + 1] = ((lmu[ro] >> 1) + diff) * 2;
>> +
>> +			/* Init smu[i+1] with 0 */
>> +			for (k = 0; k < num; k++)
>> +				smu[(i + 1) * num + k] = 0;
>> +
>> +			/* Compute smu[i+1] */
>> +			for (k = 0; k <= lmu[ro] >> 1; k++) {
>> +				int16_t a, b, c;
>> +
>> +				if (!(smu[ro * num + k] && dmu[i]))
>> +					continue;
>> +				a = readw(index_of + dmu[i]);
>> +				b = readw(index_of + dmu[ro]);
>> +				c = readw(index_of + smu[ro * num + k]);
>> +				tmp = a + (cw_len - b) + c;
>> +				a = readw(alpha_to + tmp % cw_len);
>> +				smu[(i + 1) * num + (k + diff)] = a;
>> +			}
>> +
>> +			for (k = 0; k <= lmu[i] >> 1; k++)
>> +				smu[(i + 1) * num + k] ^= smu[i * num + k];
>> +		}
>> +
>> +		/* End Computing Sigma (Mu+1) and L(mu) */
>> +		/* In either case compute delta */
>> +		delta[i + 1] = (mu[i + 1] * 2 - lmu[i + 1]) >> 1;
>> +
>> +		/* Do not compute discrepancy for the last iteration */
>> +		if (i >= cap)
>> +			continue;
>> +
>> +		for (k = 0; k <= (lmu[i + 1] >> 1); k++) {
>> +			tmp = 2 * (i - 1);
>> +			if (k == 0) {
>> +				dmu[i + 1] = si[tmp + 3];
>> +			} else if (smu[(i + 1) * num + k] && si[tmp + 3 - k]) {
>> +				int16_t a, b, c;
>> +				a = readw(index_of +
>> +						smu[(i + 1) * num + k]);
>> +				b = si[2 * (i - 1) + 3 - k];
>> +				c = readw(index_of + b);
>> +				tmp = a + c;
>> +				tmp %= cw_len;
>> +				dmu[i + 1] = readw(alpha_to + tmp) ^
>> +					dmu[i + 1];
>> +			}
>> +		}
>> +	}
>> +
>> +	return;
>> +}
>> +
>> +static int pmecc_err_location(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct atmel_nand_host *host = nand_chip->priv;
>> +	const int cap = host->pmecc_corr_cap;
>> +	const int num = 2 * cap + 1;
>> +	int sector_size = host->pmecc_sector_size;
>> +	int err_nbr = 0;	/* number of error */
>> +	int roots_nbr;		/* number of roots */
>> +	int i;
>> +	uint32_t val;
>> +	int16_t *smu = host->pmecc_smu;
>> +
>> +	writel(PMERRLOC_DISABLE, &host->pmerrloc->eldis);
>> +
>> +	for (i = 0; i <= host->pmecc_lmu[cap + 1] >> 1; i++) {
>> +		writel(smu[(cap + 1) * num + i], &host->pmerrloc->sigma[i]);
>> +		err_nbr++;
>> +	}
>> +
>> +	val = PMERRLOC_ELCFG_NUM_ERRORS(err_nbr - 1);
>> +	if (sector_size == 1024)
>> +		val |= PMERRLOC_ELCFG_SECTOR_1024;
>> +
>> +	writel(val, &host->pmerrloc->elcfg);
>> +	writel(sector_size * 8 + host->pmecc_degree * cap,
>> +			&host->pmerrloc->elen);
>> +
>> +	while (!(readl(&host->pmerrloc->elisr) & PMERRLOC_CALC_DONE))
>> +		udelay(10);
> Can you please add some timeout here. Maybe a WATCHDOG_RESET is also
> required?

sure. Scott also point this.

>
>> +
>> +	roots_nbr = (readl(&host->pmerrloc->elisr) & PMERRLOC_ERR_NUM_MASK)
>> +			>> 8;
>> +	/* Number of roots == degree of smu hence <= cap */
>> +	if (roots_nbr == host->pmecc_lmu[cap + 1] >> 1)
>> +		return err_nbr - 1;
>> +
>> +	/* Number of roots does not match the degree of smu
>> +	 * unable to correct error */
>> +	return -1;
>> +}
>> +
>> +static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>> +		int sector_num, 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, pos;
>> +	uint32_t tmp;
>> +	uint8_t err_byte;
>> +
>> +	sector_size = host->pmecc_sector_size;
>> +
>> +	while (err_nbr) {
>> +		tmp = readl(&host->pmerrloc->el[i]) - 1;
>> +		byte_pos = tmp / 8;
>> +		bit_pos  = tmp % 8;
>> +
>> +		if (byte_pos >= (sector_size + extra_bytes))
>> +			BUG();	/* should never happen */
>> +
>> +		if (byte_pos < sector_size) {
>> +			err_byte = *(buf + byte_pos);
>> +			*(buf + byte_pos) ^= (1 << bit_pos);
>> +
>> +			pos = sector_num * host->pmecc_sector_size + byte_pos;
>> +			printk(KERN_INFO "Bit flip in data area, byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
>> +				pos, bit_pos, err_byte, *(buf + byte_pos));
>> +		} else {
>> +			/* Bit flip in OOB area */
>> +			tmp = sector_num * host->pmecc_bytes_per_sector
>> +					+ (byte_pos - sector_size);
>> +			err_byte = ecc[tmp];
>> +			ecc[tmp] ^= (1 << bit_pos);
>> +
>> +			pos = tmp + nand_chip->ecc.layout->eccpos[0];
>> +			printk(KERN_INFO "Bit flip in OOB, oob_byte_pos: %d, bit_pos: %d, 0x%02x -> 0x%02x\n",
>> +				pos, bit_pos, err_byte, ecc[tmp]);
>> +		}
>> +
>> +		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) {
>> +				printk(KERN_ERR "PMECC: Too many errors\n");
>> +				mtd->ecc_stats.failed++;
>> +				return -EIO;
>> +			} else {
>> +				pmecc_correct_data(mtd, buf_pos, ecc, i,
>> +					host->pmecc_bytes_per_sector, err_nbr);
>> +				mtd->ecc_stats.corrected += err_nbr;
>> +			}
>> +		}
>> +		pmecc_stat >>= 1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int atmel_nand_pmecc_read_page(struct mtd_info *mtd,
>> +	struct nand_chip *chip, uint8_t *buf, int page)
>> +{
>> +	struct atmel_nand_host *host = chip->priv;
>> +	int eccsize = chip->ecc.size;
>> +	uint8_t *oob = chip->oob_poi;
>> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
>> +	uint32_t stat;
>> +
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_RST);
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DISABLE);
>> +	pmecc_writel(host->pmecc, cfg, ((pmecc_readl(host->pmecc, cfg))
>> +		& ~PMECC_CFG_WRITE_OP) | PMECC_CFG_AUTO_ENABLE);
>> +
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_ENABLE);
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DATA);
>> +
>> +	chip->read_buf(mtd, buf, eccsize);
>> +	chip->read_buf(mtd, oob, mtd->oobsize);
>> +
>> +	while ((pmecc_readl(host->pmecc, sr) & PMECC_SR_BUSY))
>> +		udelay(1);
>> +
>> +	stat = pmecc_readl(host->pmecc, isr);
>> +	if (stat != 0)
>> +		if (pmecc_correction(mtd, stat, buf, &oob[eccpos[0]]) != 0)
>> +			return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static void atmel_nand_pmecc_write_page(struct mtd_info *mtd,
>> +		struct nand_chip *chip, const uint8_t *buf)
>> +{
>> +	struct atmel_nand_host *host = chip->priv;
>> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
>> +	int i, j;
>> +
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_RST);
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DISABLE);
>> +
>> +	pmecc_writel(host->pmecc, cfg, (pmecc_readl(host->pmecc, cfg) |
>> +		PMECC_CFG_WRITE_OP) & ~PMECC_CFG_AUTO_ENABLE);
>> +
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_ENABLE);
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DATA);
>> +
>> +	chip->write_buf(mtd, (u8 *)buf, mtd->writesize);
>> +
>> +	while ((pmecc_readl(host->pmecc, sr) & PMECC_SR_BUSY))
>> +		udelay(1);
>> +
>> +	for (i = 0; i < host->pmecc_sector_number; i++) {
>> +		for (j = 0; j < host->pmecc_bytes_per_sector; j++) {
>> +			int pos;
>> +
>> +			pos = i * host->pmecc_bytes_per_sector + j;
>> +			chip->oob_poi[eccpos[pos]] =
>> +				readb(&host->pmecc->ecc_port[i].ecc[j]);
>> +		}
>> +	}
>> +	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +}
>> +
>> +static void atmel_pmecc_core_init(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand_chip = mtd->priv;
>> +	struct atmel_nand_host *host = nand_chip->priv;
>> +	uint32_t val = 0;
>> +	struct nand_ecclayout *ecc_layout;
>> +
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_RST);
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_DISABLE);
>> +
>> +	switch (host->pmecc_corr_cap) {
>> +	case 2:
>> +		val = PMECC_CFG_BCH_ERR2;
>> +		break;
>> +	case 4:
>> +		val = PMECC_CFG_BCH_ERR4;
>> +		break;
>> +	case 8:
>> +		val = PMECC_CFG_BCH_ERR8;
>> +		break;
>> +	case 12:
>> +		val = PMECC_CFG_BCH_ERR12;
>> +		break;
>> +	case 24:
>> +		val = PMECC_CFG_BCH_ERR24;
>> +		break;
>> +	}
>> +
>> +	if (host->pmecc_sector_size == 512)
>> +		val |= PMECC_CFG_SECTOR512;
>> +	else if (host->pmecc_sector_size == 1024)
>> +		val |= PMECC_CFG_SECTOR1024;
>> +
>> +	switch (host->pmecc_sector_number) {
>> +	case 1:
>> +		val |= PMECC_CFG_PAGE_1SECTOR;
>> +		break;
>> +	case 2:
>> +		val |= PMECC_CFG_PAGE_2SECTORS;
>> +		break;
>> +	case 4:
>> +		val |= PMECC_CFG_PAGE_4SECTORS;
>> +		break;
>> +	case 8:
>> +		val |= PMECC_CFG_PAGE_8SECTORS;
>> +		break;
>> +	}
>> +
>> +	val |= (PMECC_CFG_READ_OP | PMECC_CFG_SPARE_DISABLE
>> +		| PMECC_CFG_AUTO_DISABLE);
>> +	pmecc_writel(host->pmecc, cfg, val);
>> +
>> +	ecc_layout = nand_chip->ecc.layout;
>> +	pmecc_writel(host->pmecc, sarea, mtd->oobsize - 1);
>> +	pmecc_writel(host->pmecc, saddr, ecc_layout->eccpos[0]);
>> +	pmecc_writel(host->pmecc, eaddr,
>> +			ecc_layout->eccpos[ecc_layout->eccbytes - 1]);
>> +	/* See datasheet about PMECC Clock Control Register */
>> +	pmecc_writel(host->pmecc, clk, PMECC_CLK_133MHZ);
>> +	pmecc_writel(host->pmecc, idr, 0xff);
>> +	pmecc_writel(host->pmecc, ctrl, PMECC_CTRL_ENABLE);
>> +}
>> +
>> +static int atmel_pmecc_nand_init_params(struct nand_chip *nand)
>> +{
>> +	struct mtd_info *mtd;
>> +	struct atmel_nand_host *host;
>> +	int cap, sector_size;
>> +
>> +	mtd = &nand_info[chip_nr++];
> NAK

i will use CONFIG_SYS_NAND_SELF_INIT, so it will removed.

>
>> +	mtd->priv = nand;
>> +	host = nand->priv = &pmecc_host;
>> +
>> +	/* Detect NAND chips */
>> +	if (nand_scan_ident(mtd, 1, NULL)) {
> please change that, read later on.

ditto.

>
>> +		printk(KERN_WARNING "NAND Flash not found !\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	nand->ecc.mode = NAND_ECC_HW;
>> +	nand->ecc.calculate = NULL;
>> +	nand->ecc.correct = NULL;
>> +	nand->ecc.hwctl = NULL;
>> +
>> +	cap = host->pmecc_corr_cap = CONFIG_PMECC_CAP;
>> +	sector_size = host->pmecc_sector_size = CONFIG_PMECC_SECTOR_SIZE;
> Can't we use some information from the nand chip here? Do we need to
> hard code this paramater?

as I mention in the beginning, it seems only use as hard code.

>
>> +	host->pmecc_index_table_offset = CONFIG_PMECC_INDEX_TABLE_OFFSET;
>> +
>> +	printk(KERN_INFO "Initialize PMECC params, cap: %d, sector: %d\n",
>> +		 cap, sector_size);
>> +
>> +	host->pmecc = (struct pmecc_regs __iomem *) ATMEL_BASE_PMECC;
>> +	host->pmerrloc = (struct pmecc_errloc_regs __iomem *)
>> +			ATMEL_BASE_PMERRLOC;
>> +	host->pmecc_rom_base = (void __iomem *) ATMEL_BASE_ROM;
>> +
>> +	/* ECC is calculated for the whole page (1 step) */
>> +	nand->ecc.size = mtd->writesize;
>> +
>> +	/* set ECC page size and oob layout */
>> +	switch (mtd->writesize) {
>> +	case 2048:
>> +	case 4096:
>> +		host->pmecc_degree = PMECC_GF_DIMENSION_13;
>> +		host->pmecc_cw_len = (1 << host->pmecc_degree) - 1;
>> +		host->pmecc_sector_number = mtd->writesize / sector_size;
>> +		host->pmecc_bytes_per_sector = pmecc_get_ecc_bytes(
>> +			cap, sector_size);
>> +		host->pmecc_alpha_to = pmecc_get_alpha_to(host);
>> +		host->pmecc_index_of = host->pmecc_rom_base +
>> +			host->pmecc_index_table_offset;
>> +
>> +		nand->ecc.steps = 1;
>> +		nand->ecc.bytes = host->pmecc_bytes_per_sector *
>> +				       host->pmecc_sector_number;
>> +		if (nand->ecc.bytes > mtd->oobsize - 2) {
>> +			printk(KERN_ERR "No room for ECC bytes\n");
>> +			return -EINVAL;
>> +		}
>> +		pmecc_config_ecc_layout(&atmel_pmecc_oobinfo,
>> +					mtd->oobsize,
>> +					nand->ecc.bytes);
>> +		nand->ecc.layout = &atmel_pmecc_oobinfo;
>> +		break;
>> +	case 512:
>> +	case 1024:
>> +		/* TODO */
>> +		printk(KERN_ERR "Unsupported page size for PMECC, use Software ECC\n");
>> +	default:
>> +		/* page size not handled by HW ECC */
>> +		/* switching back to soft ECC */
>> +		nand->ecc.mode = NAND_ECC_SOFT;
>> +		nand->ecc.read_page = NULL;
>> +		nand->ecc.postpad = 0;
>> +		nand->ecc.prepad = 0;
>> +		nand->ecc.bytes = 0;
>> +		return 0;
>> +	}
>> +
>> +	nand->ecc.read_page = atmel_nand_pmecc_read_page;
>> +	nand->ecc.write_page = atmel_nand_pmecc_write_page;
>> +
>> +	atmel_pmecc_core_init(mtd);
>> +
>> +	return 0;
>> +}
>> +
>> +#else
>> +
>>   /* oob layout for large page size
>>    * bad block info is on bytes 0 and 1
>>    * the bytes have to be consecutives to avoid
>> @@ -234,7 +879,6 @@ static void atmel_nand_hwctl(struct mtd_info *mtd, int mode)
>>   
>>   int atmel_hw_nand_init_param(struct nand_chip *nand)
>>   {
>> -	static int chip_nr = 0;
>>   	struct mtd_info *mtd;
>>   
>>   	nand->ecc.mode = NAND_ECC_HW;
>> @@ -293,7 +937,9 @@ int atmel_hw_nand_init_param(struct nand_chip *nand)
>>   	return 0;
>>   }
>>   
>> -#endif
>> +#endif /* CONFIG_ATMEL_NAND_HW_PMECC */
>> +
>> +#endif /* CONFIG_ATMEL_NAND_HWECC */
>>   
>>   static void at91_nand_hwcontrol(struct mtd_info *mtd,
>>   					 int cmd, unsigned int ctrl)
>> @@ -343,8 +989,12 @@ int board_nand_init(struct nand_chip *nand)
>>   	nand->chip_delay = 20;
>>   
>>   #ifdef CONFIG_ATMEL_NAND_HWECC
>> +#ifdef CONFIG_ATMEL_NAND_HW_PMECC
>> +	res = atmel_pmecc_nand_init_params(nand);
>> +#else
>>   	res = atmel_hw_nand_init_param(nand);
>>   #endif
>> +#endif
>>   
>>   	return res;
>>   }
>> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
>> index 1ee7f99..8f06b14 100644
>> --- a/drivers/mtd/nand/atmel_nand_ecc.h
>> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
>> @@ -33,4 +33,115 @@
>>   #define ATMEL_ECC_NPR		0x10			/* NParity register */
>>   #define		ATMEL_ECC_NPARITY	(0xffff << 0)		/* NParity */
>>   
>> +/* Register access macros for PMECC */
>> +#define pmecc_readl(addr, reg) \
>> +	readl(&addr->reg)
>> +
>> +#define pmecc_writel(addr, reg, value) \
>> +	writel((value), &addr->reg)
>> +
>> +/* PMECC Register Definitions */
>> +#define PMECC_MAX_SECTOR_NUM			8
>> +struct pmecc_regs {
>> +	u32 cfg;		/* 0x00 PMECC Configuration Register */
>> +	u32 sarea;		/* 0x04 PMECC Spare Area Size Register */
>> +	u32 saddr;		/* 0x08 PMECC Start Address Register */
>> +	u32 eaddr;		/* 0x0C PMECC End Address Register */
>> +	u32 clk;		/* 0x10 PMECC Clock Control Register */
>> +	u32 ctrl;		/* 0x14 PMECC Control Register */
>> +	u32 sr;			/* 0x18 PMECC Status Register */
>> +	u32 ier;		/* 0x1C PMECC Interrupt Enable Register */
>> +	u32 idr;		/* 0x20 PMECC Interrupt Disable Register */
>> +	u32 imr;		/* 0x24 PMECC Interrupt Mask Register */
>> +	u32 isr;		/* 0x28 PMECC Interrupt Status Register */
>> +	u32 reserved0[5];	/* 0x2C-0x3C Reserved */
>> +
>> +	/* 0x40 + sector_num * (0x40), Redundancy Registers */
>> +	struct {
>> +		u8 ecc[44];	/* PMECC Generated Redundancy Byte Per Sector */
>> +		u32 reserved1[5];
>> +	} ecc_port[PMECC_MAX_SECTOR_NUM];
>> +
>> +	/* 0x240 + sector_num * (0x40) Remainder Registers */
>> +	struct {
>> +		u32 rem[12];
>> +		u32 reserved2[4];
>> +	} rem_port[PMECC_MAX_SECTOR_NUM];
>> +	u32 reserved3[16];	/* 0x440-0x47C Reserved */
>> +};
>> +
>> +/* For PMECC Configuration Register */
>> +#define		PMECC_CFG_BCH_ERR2		(0 << 0)
>> +#define		PMECC_CFG_BCH_ERR4		(1 << 0)
>> +#define		PMECC_CFG_BCH_ERR8		(2 << 0)
>> +#define		PMECC_CFG_BCH_ERR12		(3 << 0)
>> +#define		PMECC_CFG_BCH_ERR24		(4 << 0)
>> +
>> +#define		PMECC_CFG_SECTOR512		(0 << 4)
>> +#define		PMECC_CFG_SECTOR1024		(1 << 4)
>> +
>> +#define		PMECC_CFG_PAGE_1SECTOR		(0 << 8)
>> +#define		PMECC_CFG_PAGE_2SECTORS		(1 << 8)
>> +#define		PMECC_CFG_PAGE_4SECTORS		(2 << 8)
>> +#define		PMECC_CFG_PAGE_8SECTORS		(3 << 8)
>> +
>> +#define		PMECC_CFG_READ_OP		(0 << 12)
>> +#define		PMECC_CFG_WRITE_OP		(1 << 12)
>> +
>> +#define		PMECC_CFG_SPARE_ENABLE		(1 << 16)
>> +#define		PMECC_CFG_SPARE_DISABLE		(0 << 16)
>> +
>> +#define		PMECC_CFG_AUTO_ENABLE		(1 << 20)
>> +#define		PMECC_CFG_AUTO_DISABLE		(0 << 20)
>> +
>> +/* For PMECC Clock Control Register */
>> +#define		PMECC_CLK_133MHZ		(2 << 0)
>> +
>> +/* For PMECC Control Register */
>> +#define		PMECC_CTRL_RST			(1 << 0)
>> +#define		PMECC_CTRL_DATA			(1 << 1)
>> +#define		PMECC_CTRL_USER			(1 << 2)
>> +#define		PMECC_CTRL_ENABLE		(1 << 4)
>> +#define		PMECC_CTRL_DISABLE		(1 << 5)
>> +
>> +/* For PMECC Status Register */
>> +#define		PMECC_SR_BUSY			(1 << 0)
>> +#define		PMECC_SR_ENABLE			(1 << 4)
>> +
>> +/* PMERRLOC Register Definitions */
>> +struct pmecc_errloc_regs {
>> +	u32 elcfg;	/* 0x00 Error Location Configuration Register */
>> +	u32 elprim;	/* 0x04 Error Location Primitive Register */
>> +	u32 elen;	/* 0x08 Error Location Enable Register */
>> +	u32 eldis;	/* 0x0C Error Location Disable Register */
>> +	u32 elsr;	/* 0x10 Error Location Status Register */
>> +	u32 elier;	/* 0x14 Error Location Interrupt Enable Register */
>> +	u32 elidr;	/* 0x08 Error Location Interrupt Disable Register */
>> +	u32 elimr;	/* 0x0C Error Location Interrupt Mask Register */
>> +	u32 elisr;	/* 0x20 Error Location Interrupt Status Register */
>> +	u32 reserved0;	/* 0x24 Reserved */
>> +	u32 sigma[25];	/* 0x28-0x88 Error Location Sigma Registers */
>> +	u32 el[24];	/* 0x8C-0xE8 Error Location Registers */
>> +	u32 reserved1[5];	/* 0xEC-0xFC Reserved */
>> +};
>> +
>> +/* For Error Location Configuration Register */
>> +#define		PMERRLOC_ELCFG_SECTOR_512	(0 << 0)
>> +#define		PMERRLOC_ELCFG_SECTOR_1024	(1 << 0)
>> +#define		PMERRLOC_ELCFG_NUM_ERRORS(n)	((n) << 16)
>> +
>> +/* For Error Location Disable Register */
>> +#define		PMERRLOC_DISABLE		(1 << 0)
>> +
>> +/* For Error Location Interrupt Status Register */
>> +#define		PMERRLOC_ERR_NUM_MASK		(0x1f << 8)
>> +#define		PMERRLOC_CALC_DONE		(1 << 0)
>> +
>> +/* Galois field dimension */
>> +#define PMECC_GF_DIMENSION_13			13
>> +#define PMECC_GF_DIMENSION_14			14
>> +
>> +#define PMECC_INDEX_TABLE_SIZE_512		0x2000
>> +#define PMECC_INDEX_TABLE_SIZE_1024		0x4000
>> +
>>   #endif
>>
> Not really nice code but it seems the kernel guys accepted it the same
> way. So I will _not_ NAK it. But please add at least a README entry
> describing the new config parameters, think about the sector size (ore
> explain me why you need another config) and please change the driver
> initialization to the SELF_INIT as mentioned in another mail. You use
> again the nand_scan_indent() inside your xx_nand_init_params().
> Please rewrite the board_nand_init() as shown in doc/README.nand, maybe
> copy the stub from mtd/nand.c's nand_init() and nand_init_chip(). Call
> in your own atmel_nand_init_chip() nand_scan_ident() and after that
> setup the parameters for eighter ECC or PMECC. Always hand over the
> struct nand_chip and do not work with the nand_chip array in the
> xx_init_param() functions.

I will send next version according to yours and Scott's suggestion. 
Thank you.

>
> Best regards
>
> Andreas Bießmann

Best Regards,
Josh Wu

On 08/21/2012 05:37 AM, Josh Wu wrote:
> Hi, Andreas
> 
> On 8/17/2012 5:24 PM, Andreas Bießmann wrote:
>> can you please add some README entry describing these new config
>> parameters?
>> Namely CONFIG_ATMEL_NAND_HW_PMECC, CONFIG_PMECC_CAP,
>> CONFIG_PMECC_SECTOR_SIZE (can't this be derived from some already
>> available NAND information?) and CONFIG_PMECC_INDEX_TABLE_OFFSET.
> 
> OK, I will add a README file to explain all the parameters.
> this CONFIG_PMECC_SECTOR_SIZE means how many bytes to generate out PMECC
> code. It only can be 512 and 1024.
> So for a nand chip whose page size is 2048, if CONFIG_PMECC_SECTOR_SIZE
> is set as 512, then PMECC will generate PMECC code for each 512 bytes.
> 
> I think it cannot be derived from nand information.

So this is basically nand->ecc.size?  While this can't be directly read
from the chip, usually there's a convention for a given type of NAND
chip on a given controller.  Do you really need to support both 512 and
1024 for any single chip?

Why do you set nand->ecc.size to mtd->writesize if that isn't the actual
ECC chunk size?

-Scott

Hi, Scott

On 8/22/2012 4:39 AM, Scott Wood wrote:
> On 08/21/2012 05:37 AM, Josh Wu wrote:
>> Hi, Andreas
>>
>> On 8/17/2012 5:24 PM, Andreas Bießmann wrote:
>>> can you please add some README entry describing these new config
>>> parameters?
>>> Namely CONFIG_ATMEL_NAND_HW_PMECC, CONFIG_PMECC_CAP,
>>> CONFIG_PMECC_SECTOR_SIZE (can't this be derived from some already
>>> available NAND information?) and CONFIG_PMECC_INDEX_TABLE_OFFSET.
>> OK, I will add a README file to explain all the parameters.
>> this CONFIG_PMECC_SECTOR_SIZE means how many bytes to generate out PMECC
>> code. It only can be 512 and 1024.
>> So for a nand chip whose page size is 2048, if CONFIG_PMECC_SECTOR_SIZE
>> is set as 512, then PMECC will generate PMECC code for each 512 bytes.
>>
>> I think it cannot be derived from nand information.

I think I am not make me clear in previous email. let me try again :)

For the PMECC hardware, it support to read/write one data page of nand 
in one step. The size of the data page should be multiple(max is 8) of 
PMECC sector. The PMECC sector can only be 512 or 1024.

So the PMECC can read/write max 8192 bytes (max data page size: 8 x 
1024) in one step. And generate the ecc bytes (8 x 4) for this page if 
the error correct capability is 2 bits.

But the internal thing is those ecc bytes is split into 8 parts and each 
part is for corresponding sector.

> So this is basically nand->ecc.size?

I don't think nand->ecc.size is the CONFIG_PMECC_SECTOR_SIZE. Since for 
my understanding, the ecc.size is the data bytes per ecc step. In my 
code, PMECC can read/write one page in one step.
So I set nand->ecc.size to the page size.

> While this can't be directly read
> from the chip, usually there's a convention for a given type of NAND
> chip on a given controller.  Do you really need to support both 512 and
> 1024 for any single chip?

hmm. So maybe I can set default PMECC sector size to 512 if nand flash 
page is smaller than 4096 (8x512) bytes. And set it to 1024 only when 
nand flash page is larger than 4096.
But in other side, if CONFIG_PMECC_SECTOR_SIZE is defined, then driver 
will use it.

>
> Why do you set nand->ecc.size to mtd->writesize if that isn't the actual
> ECC chunk size?

As mentioned in above, the ecc.size is the data bytes per ecc step. In 
PMECC code, PMECC can read/write one page in one step.
So I set nand->ecc.size to the page size not the actual inside ECC chunk 
size: CONFIG_PMECC_SECTOR_SIZE

> -Scott
>
>
Thanks.
Best Regards,
Josh Wu