Re: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic

[Boris Brezillon]

On Mon, 19 Aug 2019 09:03:38 +0000
"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@xxxxxxxxxx> wrote:

> >   
> > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > >  {
> > > +	const struct spi_mem_op *op;
> > >  	u8 *id = spinand->id.data;
> > > -	int ret;
> > >
> > >  	/*
> > >  	 * Micron SPI NAND read ID need a dummy byte,
> > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct  
> > spinand_device *spinand)  
> > >  	if (id[1] != SPINAND_MFR_MICRON)
> > >  		return 0;
> > >
> > > -	ret = spinand_match_and_init(spinand, micron_spinand_table,
> > > -				     ARRAY_SIZE(micron_spinand_table),  
> > id[2]);
> > 
> > I am not sure this is the right solution. I would keep this call and
> > overwrite what you need to overwrite with the fixup hook.
> >   

I'm definitely not comfortable with this whole "rely on ONFi
param-page" thing. Vendors have proven to get it wrong from time to
time, so before we do that, I'd like to make sure all currently
supported Micron NANDs (looks like we only support MT29F2G01ABAGD, so
that shouldn't be hard) expose the right thing there. For instance, are
we sure the ECC layout is always the same, and if not, do we have a
reliable way to extract that?

> 
> Then, I will have dummy structure like below.
> 
> static const struct spinand_info micron_spinand_table[] = {                      
>         SPINAND_INFO(NULL, 0,                                                                    
>                      NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),           
>                      NAND_ECCREQ(0, 0),                                                                       
>                      SPINAND_INFO_OP_VARIANTS(&read_cache_variants,              
>                                               &write_cache_variants,             
>                                               &update_cache_variants),           
>                      0,                                                                                         
>                      SPINAND_ECCINFO(&micron_ooblayout_ops,                      
>                                      micron_ecc_get_status)),                    
> };   

> 
> Let me know if you are thinking for different approach.

Exposing dummy entries is useless. If you're entirely sure all Micron
SPI NANDs have a valid ONFi param page, then no need to use the
ID-based detection. But as I said above, I feel param-page-based
detection is going to be as messy as SFDP-based detection is for SPI
NORs. Vendors tend to make mistakes which we have to fix to make
things work. ID-based detection is much more reliable in this regard,
as long as we don't have ID collisions :P.
Plus, it looks like only a few manufacturers decided to use ONFi param
pages to expose SPI NAND info (AFAICT, only Micron and Macronix do
that), which is not surprising since the ONFi param page has been
created to describe parallel NANDs not SPI NANDs (if you look closely
enough, you'll notice that some fields are meaningless for SPI NANDs).