Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Arnaud Mouiche <arnaud.mouiche@gmail.com>]

On 03/03/2017 11:00, Boris Brezillon wrote:
> On Fri, 3 Mar 2017 10:37:04 +0100
> Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
>
>> On 03/03/2017 10:28, Boris Brezillon wrote:
>>> On Fri, 3 Mar 2017 16:37:55 +0800
>>> Peter Pan <peterpansjtu@gmail.com> wrote:
>>>
>>> [..]
>>>> BTW, there is another question. read id method is not unique. Micron spi nand
>>>> need a dummy byte before reading ID while some vendors don't. Now I define
>>>> vendor alias in DTS and use this info to choose right manufacture ops. Do you
>>>> have a better idea?
>>> Ouch. That's bad news. How about letting the manufacturer code read the
>>> ID and detect the NAND?
>>>
>>> That means you'll iterate over all manufacturer entries in the
>>> manufacturer table and call ->detect(). The ->detect() hook will be
>>> responsible for reading the ID (with the proper read-id sequence) and
>>> initialize the NAND parameters.
>>>
>>> If we find a common pattern between different vendors, we can then
>>> provide default helpers for the read-id and/or detect implementation.
>> A effective way will be to read the up to 4 bytes of ID response, and
>> ask every manufacturer to provide an ID and mask to compare with.
>> Here is the list of ID/MASK I have compiled.
>>
>>           "MT29F1G01AAADD",
>>           .id = 0x002C1200,  .id_mask = 0x00FFFF00,
>>
>>           "MT29F2G01AAAED",
>>
>>           "MT29F4G01AAADD",
>>           .id = 0x002C3200,  .id_mask = 0x00FFFF00,
>>
>>           "GD5F1GQ4xC", /* version U (3.3V) or R (1.8) */
>>           .id = 0xC8A14800,  .id_mask = 0xffefff00, /* 0xC8A148 or
>> 0xC8B148 */
>>
>>           "GD5F2GQ4xC", /* version U (3.3V) or R (1.8) */
>>           .id = 0xC8A24800,  .id_mask = 0xffefff00, /* 0xC8A248 or
>> 0xC8B248 */
>>
>>           "GD5F1GQ4xBYIG", /* version U (3.3V) or R (1.8) */
>>           .id = 0x00C8D100,  .id_mask = 0x00ffef00, /* C8D1 or C8C1 */
>>
>>           "GD5F2GQ4xBYIG", /* version U (3.3V) or R (1.8) */
>>           .id = 0x00C8D200,  .id_mask = 0x00ffef00, /* C8D2 or C8C2 */
>>
>>           "F50L1G41A", /* ESMT */
>>           .id = 0x00C8217F,  .id_mask = 0x00ffffff,
>>
>>           "W25N01GVZEIG", /* Winbond */
>>           .id = 0x00EFAA21,  .id_mask = 0x00ffffff,
>>
>>           "MX35LF1GE4AB", /* Micronix */
>>           .id = 0x00C21200,  .id_mask = 0x00ffff00,
> I'm not a big fan of this approach. See how each vendor seems to have
> its own scheme, and we're not even sure they will use the same for
> their next chips. That's what happened with raw NANDs, and the NAND ID
> parsing just became a huge pile of hacks like that:
>
> 	if(vendorX and revisionY) then id-should-decoded-like-that;
>
> By letting the detection process to manufacturer code, we just get rid
> of this complexity in the core, which is a good thing IMO.

I agree, but only if you do the SPI command once, and ask each vendor 
"->detect()" to check the content of the returned data.
Otherwise, it may take some [long] time to finally know the real ID of 
one chip.

Arnaud