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

[Florian Fainelli <ffainelli@freebox.fr>]

Hello,

On Friday 13 August 2010 09:25:03 Matthieu CASTET wrote:
> 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.

Maybe we should add a crc16_le to lib/crc16.c?

> 
> >>> +
> >>> +   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
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/