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

[Bo Shen <voice.shen@atmel.com>]

Hi Andreas,

On 10/25/2014 06:22 AM, Andreas Bie?mann wrote:
> 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 SAMA5D4 SoC, the PMECC table is only seen by the ROM loader. So, 
for atbootstrap, we also need to generate the PMECC table and put it in 
DDR2 SDRAM.
   We can use the PMECC generate by atbootstrap, however we must to 
maintain the address where to put the PMECC table, it will easily cause 
issues, so we try to regenerate the PMECC table also in u-boot.

> On 16.09.14 09:57, Bo Shen wrote:
>> From: Josh Wu <josh.wu@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@atmel.com>
>> Signed-off-by: Bo Shen <voice.shen@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?

OK. I will change it.

>> +
>> +	/* 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[]?

I will consider that.

>> +
>> +	/*
>> +	 * 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?

I will try to check the table would be usable in BCH library code.

>> +}
>> +#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 ...

I test with sama5d3xek board, it is OK. No problem found.

>> +	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
>

Best Regards,
Bo Shen