Re: [PATCH 3/3] NAND: add support for reading ONFI parameters from NAND device

[Matthieu CASTET <matthieu.castet@parrot.com>]

Hello,

Florian Fainelli a écrit :
> Hello Brian,
> 
> Le Thursday 12 August 2010 20:57:42, Brian Norris a écrit :
>> Hello,
>>
>> I've got a few comments and a patch; I cannot test them, though,
>> just build.
>>
>> On 08/12/2010 05:53 AM, Florian Fainelli wrote:
>>> +static u16 onfi_crc(u16 crc, unsigned char const *p, size_t len)
>>> +{
>>> +   int i;
>>> +   while (len--) {
>>> +           crc ^= *p++<<  8;
>>> +           for (i = 0; i<  8; i++)
>>> +                   crc = (crc<<  1) ^ ((crc&  0x8000) ? 0x8005 : 0);
>>> +   }
>>> +   return crc;
>>> +}
>> Can this function safely be replaced by the library function crc16?
>> #include <linux/crc16.h>
>> u16 crc16(u16 crc, u8 const *buffer, size_t len);
> 
> Certainly, thanks for noticing.
No it is not the same crc16 endianness, one is big the other is little.



>>
>>> +
>>> +   chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
>>> +
>>> +   /* Read entire ID string */
>>> +
>>> +   for (i = 0; i<  8; i++)
>>> +           id_data[i] = chip->read_byte(mtd);
>>> +
>>>
>>>     if (!type->name)
>>>
>>>             return ERR_PTR(-ENODEV);
>> Do we really need a third chance to read the ID bytes? It seems like we
>> can just read the whole string the second time instead of shortening it
>> to two bytes and waiting to reread all 8 bytes until after the ONFI scan.
Well onfi define only 2 bytes for id string, I though it was safer to 
not read beyond specification for onfi nand.



>>
>>> +                                   if (val&  (1<<  1))
>>> +                                           chip->onfi_version = 10;
>>> +                                   else if (val&  (1<<  2))
>>> +                                           chip->onfi_version = 20;
>>> +                                   else if (val&  (1<<  3))
>>> +                                           chip->onfi_version = 21;
>>> +                                   else
>>> +                                           chip->onfi_version = 22;
>> I cannot currently test ONFI on a real chip, but shouldn't the order of
>> these conditionals be switched? It seems possible that the bits could be
>> set high for more the one version (e.g., a chip supports 1.0 and 2.0, so
>> we have val = 00000110 (binary), so the current logic would succeed at 1.0,
>> not realizing that it supports 2.0. Again, I don't know about the actual
>> behavior in a real chip, but anyway, it seems harmless to reverse this.
> 
> I think you are right about this. We only have ONFI 1.0 compliant chips right
> now, so we can't know for sure, but as you say, this is harmless.
ok


Matthieu