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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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.