[PATCH v2 4/5] mtd: nand: parse out the JEDEC compliant nand

[Brian Norris]

Hi,

On Fri, Feb 14, 2014 at 03:08:54PM +0800, Huang Shijie wrote:
> On Tue, Feb 11, 2014 at 12:32:24PM -0800, Brian Norris wrote:
> > On Sat, Feb 08, 2014 at 02:04:00PM +0800, Huang Shijie wrote:
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3163,6 +3163,88 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> > >  }
> > >  
> > >  /*
> > > + * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
> > > + */
> > > +static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> > > +					int *busw)
> > > +{
> > > +	struct nand_jedec_params *p = &chip->jedec_params;
> > > +	struct jedec_ecc_info *ecc;
> > > +	int val;
> > > +	int i, j;
> > > +
> > > +	/* Try JEDEC for unknown chip or LP */
> > > +	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
> > 
> > Where did you get this READID column address? I may be misreading
> 
> I get the column from the Toshiba and Micron datasheet.
> the JEDEC really does not mention it.

OK, that's fine. It's still a bit strange the JEDEC doesn't mention it.

> > something, but I don't even find this in the JESD230A spec.
> > 
> > > +	if (chip->read_byte(mtd) != 'J' || chip->read_byte(mtd) != 'E' ||
> > > +		chip->read_byte(mtd) != 'D' || chip->read_byte(mtd) != 'E' ||
> > > +		chip->read_byte(mtd) != 'C')
> > > +		return 0;
> > > +
> > > +	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40, -1);
> > 
> > Same here. I don't actually find this column address in the spec. I
> ditto.
> 
> > really hope this is specified somewhere.
> > 
> > Also, now that we may have non-zero address to NAND_CMD_PARAM, do we
> > need to make this adjustment so it can be used on x16 buswidth? Perhaps this diff?
> 
> sorry, I really do not know the x16 issue :(
> 
> the gpmi is 8bit.

That's fine, I expected that. But I think that in the absence of
evidence otherwise, we should always send the address for NAND_CMD_PARAM
as if it's only on the lower 8 bits, not a x16-width column address. Can
you squash in my diff below, or else I can send it as a separate patch?

> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 84cca611c2fd..1b1ad3d25336 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -921,7 +921,7 @@ static inline bool nand_is_slc(struct nand_chip *chip)
> >   */
> >  static inline int nand_opcode_8bits(unsigned int command)
> >  {
> > -	return command == NAND_CMD_READID;
> > +	return command == NAND_CMD_READID || command == NAND_CMD_PARAM;
> >  }
> >  
> >  /* return the supported JEDEC features. */
> > 
> > > +	for (i = 0; i < 3; i++) {
> > > +		for (j = 0; j < sizeof(*p); j++)
> > > +			((uint8_t *)p)[j] = chip->read_byte(mtd);
> > > +
> > > +		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> > > +				le16_to_cpu(p->crc))
> > > +			break;
> > > +	}
> > > +
> > > +	if (i == 3) {
> > > +		pr_err("Could not find valid JEDEC parameter page; aborting\n");
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Check version */
> > > +	val = le16_to_cpu(p->revision);
> > > +	if (val & (1 << 2))
> > > +		chip->jedec_version = 10;
> > > +	else if (val & (1 << 1))
> > > +		chip->jedec_version = 1; /* vendor specific version */
> > 
> > How did you determine that bit[1] means version 1 (or v0.1)? It's
> 
> The bit[1] is the vendor specific version. 
> Some Micron chips uses the bit[1], such as MT29F64G08CBAB.

I noticed a similar thing with my Micron datasheet.

> If we do not use the 1 for the it, do you have any suggestion
> for the vendor specific version?
> 
> 
> > unclear to me how to interpret the revision bitfield here, but it
> > appears that bit[1] is not really useful to us yet, unless we're using
> > vendor specific parameter page.
> 
> > 
> > What do we do if we see bit[1] (vendor specific parameter page) but not
> > bit[2] (parameter page revision 1.0 / standard revision 1.0)? I'm
> > thinking that we should just ignore bit[1] entirely for now, and if the
> > flash is missing bit[2], then we can't support it.
> 
> some Micron chips do use the bit[1]. i think that's why the JEDEC say:
> "supports vendor specific parameter page" for the bit[1].
> 
> We'd better keep the support for the bit[1].

OK. But we need to be careful that whatever this "vendor specific
parameter page" is, it's always compatible with the spec we're referring
to (v1.0). Otherwise, we can't rely on bit[1] to tell us anything
specific. This really seems like some sloppiness on either Micron or
JEDEC.

[snip]

Thanks,
Brian