[U-Boot] [PATCH 1/3] atmel_nand: if don't have gf table in rom code we will build it runtime

[Andreas Bießmann]

Hi Bo,

before diving into that code I have a question. What will be the boot
mode for devices without integrated PMECC table? Can the first stage
loader be read with PMECC mode?
The question arises while thinking about this patch. We have generally
two options here:
 a) pre-generated PMECC table included in u-boot
 b) runtime generated PMECC table in RAM (this solution)

While a) will bloat the u-boot binary b) will require some RAM to hold
the table. If I understood correctly we will malloc at most 128 KiB for
the PMECC table ... Ok option a) is not an option ;)
Here my question arises, how can the ROM loader allocate 128 KiB? Do
these devices have so many SRAM? Or am I wrong?

On 16.09.14 09:57, Bo Shen wrote:
> From: Josh Wu <josh.wu at atmel.com>
> 
> Add a macro NO_GALOIS_TABLE_IN_ROM. If it is defined we will build a
> runtime pmecc galois table.
> 
> Signed-off-by: Josh Wu <josh.wu at atmel.com>
> Signed-off-by: Bo Shen <voice.shen at atmel.com>
> ---
> 
>  drivers/mtd/nand/atmel_nand.c | 127 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 126 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index e73834d..7cf7b39 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -761,6 +761,108 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
>  }
>  #endif
>  
> +#if defined(NO_GALOIS_TABLE_IN_ROM)
> +static uint16_t *pmecc_galois_table;
> +static int pmecc_build_galois_table(unsigned int mm,
> +		int16_t *index_of, int16_t *alpha_to)
> +{
> +	unsigned int i, mask, nn;
> +	unsigned int p[15];
> +
> +	nn = (1 << mm) - 1;
> +	/* set default value */
> +	for (i = 1; i < mm; i++)
> +		p[i] = 0;

memset(p[1], 0, mm-1); // or even the whole field?

> +
> +	/* 1 + X^mm */
> +	p[0]  = 1;
> +	p[mm] = 1;
> +
> +	/* others  */
> +	switch (mm) {
> +	case 3:
> +	case 4:
> +	case 6:
> +	case 15:
> +		p[1] = 1;
> +		break;
> +	case 5:
> +	case 11:
> +		p[2] = 1;
> +		break;
> +	case 7:
> +	case 10:
> +		p[3] = 1;
> +		break;
> +	case 8:
> +		p[2] = 1;
> +		p[3] = 1;
> +		p[4] = 1;
> +		break;
> +	case 9:
> +		p[4] = 1;
> +		break;
> +	case 12:
> +		p[1] = 1;
> +		p[4] = 1;
> +		p[6] = 1;
> +		break;
> +	case 13:
> +		p[1] = 1;
> +		p[3] = 1;
> +		p[4] = 1;
> +		break;
> +	case 14:
> +		p[1] = 1;
> +		p[6] = 1;
> +		p[10] = 1;
> +		break;
> +	default:
> +		/* Error */
> +		return -EINVAL;
> +	}
> +
> +	/* Build alpha ^ mm it will help to generate the field (primitiv) */
> +	alpha_to[mm] = 0;
> +	for (i = 0; i < mm; i++)
> +		if (p[i])
> +			alpha_to[mm] |= 1 << i;

In fact p[] is a bit-field which is then copied to alpha_to[mm], why not
use alpha_to[mm] in the switch-case and eliminate p[]?

> +
> +	/*
> +	 * Then build elements from 0 to mm - 1. As degree is less than mm
> +	 * so it is just a logical shift.
> +	 */
> +	mask = 1;
> +	for (i = 0; i < mm; i++) {
> +		alpha_to[i] = mask;
> +		index_of[alpha_to[i]] = i;
> +		mask <<= 1;
> +	}
> +
> +	index_of[alpha_to[mm]] = mm;
> +
> +	/* use a mask to select the MSB bit of the LFSR */
> +	mask >>= 1;
> +
> +	/* then finish the building */
> +	for (i = mm + 1; i <= nn; i++) {
> +		/* check if the msb bit of the lfsr is set */
> +		if (alpha_to[i - 1] & mask)
> +			alpha_to[i] = alpha_to[mm] ^
> +				((alpha_to[i - 1] ^ mask) << 1);
> +		else
> +			alpha_to[i] = alpha_to[i - 1] << 1;
> +
> +		index_of[alpha_to[i]] = i % nn;
> +	}
> +
> +	/* index of 0 is undefined in a multiplicative field */
> +	index_of[0] = -1;
> +
> +	return 0;

The whole function is so longish and complicated ... I don't really like
it. Maybe eliminating p[] makes it better?

> +}
> +#endif
> +
>  static int atmel_pmecc_nand_init_params(struct nand_chip *nand,
>  		struct mtd_info *mtd)
>  {
> @@ -808,11 +910,18 @@ static int atmel_pmecc_nand_init_params(struct nand_chip *nand,
>  	sector_size = host->pmecc_sector_size;
>  
>  	/* TODO: need check whether cap & sector_size is validate */
> -
> +#if defined(NO_GALOIS_TABLE_IN_ROM)
> +	/*
> +	 * As pmecc_rom_base is the begin of the gallois field table, So the
> +	 * index offset just set as 0.
> +	 */
> +	host->pmecc_index_table_offset = 0;
> +#else
>  	if (host->pmecc_sector_size == 512)
>  		host->pmecc_index_table_offset = ATMEL_PMECC_INDEX_OFFSET_512;
>  	else
>  		host->pmecc_index_table_offset = ATMEL_PMECC_INDEX_OFFSET_1024;
> +#endif
>  
>  	MTDDEBUG(MTD_DEBUG_LEVEL1,
>  		"Initialize PMECC params, cap: %d, sector: %d\n",
> @@ -821,7 +930,23 @@ static int atmel_pmecc_nand_init_params(struct nand_chip *nand,
>  	host->pmecc = (struct pmecc_regs __iomem *) ATMEL_BASE_PMECC;
>  	host->pmerrloc = (struct pmecc_errloc_regs __iomem *)
>  			ATMEL_BASE_PMERRLOC;
> +#if defined(NO_GALOIS_TABLE_IN_ROM)
> +	/* Set pmecc_rom_base as the begin of gf table */
> +	int size = host->pmecc_sector_size == 512 ?
> +		PMECC_INDEX_TABLE_SIZE_512 :
> +		PMECC_INDEX_TABLE_SIZE_1024;
> +	pmecc_galois_table = malloc(2 * size * sizeof(uint16_t));

I fear this will not work with current SPL implementation ...

> +	if (!pmecc_galois_table)
> +		return -ENOMEM;
> +
> +	host->pmecc_rom_base = pmecc_galois_table;
> +	pmecc_build_galois_table((sector_size == 512) ? PMECC_GF_DIMENSION_13 :
> +			PMECC_GF_DIMENSION_14,
> +			host->pmecc_rom_base,
> +			host->pmecc_rom_base + (size * sizeof(int16_t)));
> +#else
>  	host->pmecc_rom_base = (void __iomem *) ATMEL_BASE_ROM;
> +#endif
>  
>  	/* ECC is calculated for the whole page (1 step) */
>  	nand->ecc.size = mtd->writesize;
> 

Best regards

Andreas Bießmann