[U-Boot] [PATCH V2 2/4] mtd/NAND: Add FSMC driver support

[Amit Virdi <amit.virdi@st.com>]

Hi Scott,

On 5/16/2012 2:44 AM, Scott Wood wrote:
> On 05/07/2012 02:26 AM, Amit Virdi wrote:
>> +	while (num_err--) {
>> +		change_bit(0,&err_idx[i]);
>> +		change_bit(1,&err_idx[i]);
>> +
>> +		if (err_idx[i]<  512 * 8) {
>> +			change_bit(err_idx[i], dat);
>> +			i++;
>> +		}
>> +	}
>
> Is it normal to not count bit flips in the ECC itself?
>

Correcting bit flip in ECC isn't of any use, so we skipped it.

Otherwise also, the ECC and the data area are not contiguous here as 
they are at different parts of RAM so we couldn't have done
	if (err_idx[i]<  (512+13) * 8) {
here.

>> +{
>> +	u_int ecc_tmp;
>> +
>> +	switch (fsmc_version) {
>> +	case FSMC_VER8:
>> +		/* Busy waiting for ecc computation to finish for 512 bytes */
>> +		while (!(readl(&fsmc_regs_p->sts)&  FSMC_CODE_RDY))
>> +			;
>
> Timeout?
>

Ok, I'll implement 1 sec timeout.

>> +	/*
>> +	 * ecc_oob is intentionally taken as u16. In 16bit devices, we end up
>> +	 * reading 14 bytes (7 words) from oob. The local array is to maintain
>> +	 * word alignment
>> +	 */
>> +	uint16_t ecc_oob[7];
>> +	uint8_t *oob = (uint8_t *)&ecc_oob[0];
>
> Please use a union, or better an explicit alignment attribute.
>

Ok.

>> +enum {
>> +	FSMC_VER1 = 1,
>> +	FSMC_VER2,
>> +	FSMC_VER3,
>> +	FSMC_VER4,
>> +	FSMC_VER5,
>> +	FSMC_VER6,
>> +	FSMC_VER7,
>> +	FSMC_VER8,
>> +};
>
> Is this really necessary?
>

No. I'll remove it.

Thanks
Amit Virdi