Patchwork NAND: add support for reading ONFI parameters from NAND device

login
register
mail settings
Submitter Florian Fainelli
Date July 28, 2010, 10:47 p.m.
Message ID <201007290047.06394.ffainelli@freebox.fr>
Download mbox | patch
Permalink /patch/60180/
State New
Headers show

Comments

Florian Fainelli - July 28, 2010, 10:47 p.m.
A nand_chip which has valid ONFI parameters gets its options field updated
with the NAND_ONFI flag. In that case both the ONFI version (in BCD format)
as well as the complete page parameters is available in the struct nand_chip.
This allows for better detection of some new devices, as well as fine tuning of
NAND driver timings. This patch only adds support for ONFI 1.0 parameters.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
Signed-off-by: Florian Fainelli <ffainelli@freebox.fr>
---
Brian Norris - July 28, 2010, 11:38 p.m.
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index a81b185..ad7f58f 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
<snip>
> @@ -190,6 +194,9 @@ typedef enum {
>   /* Device behaves just like nand, but is readonly */
>   #define NAND_ROM		0x00000800
> 
> +/* Chip supports ONFI */
> +#define NAND_ONFI		0x00001000

I've been wondering: how independent are the flags in include/linux/mtd/bbm.h
and nand.h? I've working on some patches dealing with various such flags. For
instance, I know that the following patch dealt with a potential conflict
between flags in bbm.h and nand.h:
http://lists.infradead.org/pipermail/linux-mtd/2010-June/030703.html

I don't know if there's a possibility of conflict between NAND_BBT_WRITE (bbm.h)
and your new NAND_ONFI (nand.h); both are 0x00001000. I know *some* options are
written into nand_chip->options and later copied onto the options in
nand_bbt_descr->options for BBT usage, e.g., I just rewrote part of this as a
new function nand_create_default_bbt_descr() in nand_bbt.c:
http://lists.infradead.org/pipermail/linux-mtd/2010-July/030911.html

Other pieces of the code perform similar functions at the moment.

Brian
Matthieu CASTET - July 29, 2010, 7:54 a.m.
Hi,

Florian Fainelli a écrit :
> A nand_chip which has valid ONFI parameters gets its options field updated
> with the NAND_ONFI flag. In that case both the ONFI version (in BCD format)
> as well as the complete page parameters is available in the struct nand_chip.
> This allows for better detection of some new devices, as well as fine tuning of
> NAND driver timings. This patch only adds support for ONFI 1.0 parameters.
> 
> Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
> Signed-off-by: Florian Fainelli <ffainelli@freebox.fr>
> ---
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 4a7b864..c255cec 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2773,6 +2773,83 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
>  }
>  

> +
> +/*
> + * Check whether flash support ONFI and read ONFI parameters in that
> + * case
> + */
> +static void nand_read_onfi(struct mtd_info *mtd, struct nand_chip *chip)
> +{
> +	struct nand_onfi_params *p;
> +	uint8_t sig[4];
> +	uint16_t val;
> +
> +	if (!chip->cmdfunc || !chip->read_buf)
> +		return;
> +
> +	/* read ONFI signature */
> +	chip->cmdfunc(mtd, NAND_CMD_READID, NAND_ADDR_ONFI_ID, -1);
> +	chip->read_buf(mtd, sig, sizeof(sig));
> +
> +	if (memcmp(sig, "ONFI", sizeof(sig)))
> +		return;
> +
> +	/* ONFI seems supported */
> +	chip->cmdfunc(mtd, NAND_CMD_READ_ONFI_PARAMS, 0, -1);
> +	p = &chip->onfi_params;
> +	chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> +
> +	/* recheck signature */
> +	if (memcmp(p->sig, "ONFI", sizeof(p->sig))) {
> +		printk(KERN_INFO "%s: bad ONFI params signature\n", __func__);
> +		return;
> +	}
You need to check crc. onfi param can be corrupted (bitflip ?) and 
should try at least 3 page.


Also you don't handle endianness (integer are little endian) for value 
in nand_onfi_params.

Also I am not sure it is worth to export all the onfi param. Fill the 
mtd param for page, erase, oob size and other stuff.
After all rev info and features block, manufacturer information block, 
memory organization block should only be used by mtd layer.
Only electrical parameter block can interest driver for fine tunning.


>   * Get the flash and manufacturer id and lookup if the type is supported
>   */
>  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> @@ -2958,10 +3035,19 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>  	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
>  		chip->cmdfunc = nand_command_lp;
>  
> +	nand_read_onfi(mtd, chip);
> +
>  	printk(KERN_INFO "NAND device: Manufacturer ID:"
>  	       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, dev_id,
>  	       nand_manuf_ids[maf_idx].name, type->name);
>  
> +	if (chip->options & NAND_ONFI)
> +		printk(KERN_INFO "NAND is ONFI %d.%d compliant "
> +		       "(man:%s model:%s)\n",
> +		       chip->onfi_version / 10, chip->onfi_version % 10,
> +		       chip->onfi_params.manufacturer,
> +		       chip->onfi_params.model);
> +
>  	return type;
>  }
>  
This won't work this unknown nand, and not work with some LP nand that 
doesn't provide additional id bytes.


Matthieu
Florian Fainelli - July 29, 2010, 8:10 a.m.
Hi Brian,

On Thursday 29 July 2010 01:38:01 Brian Norris wrote:
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index a81b185..ad7f58f 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> 
> <snip>
> 
> > @@ -190,6 +194,9 @@ typedef enum {
> > 
> >   /* Device behaves just like nand, but is readonly */
> >   #define NAND_ROM		0x00000800
> > 
> > +/* Chip supports ONFI */
> > +#define NAND_ONFI		0x00001000
> 
> I've been wondering: how independent are the flags in
> include/linux/mtd/bbm.h and nand.h? I've working on some patches dealing
> with various such flags. For instance, I know that the following patch
> dealt with a potential conflict between flags in bbm.h and nand.h:
> http://lists.infradead.org/pipermail/linux-mtd/2010-June/030703.html
> 
> I don't know if there's a possibility of conflict between NAND_BBT_WRITE
> (bbm.h) and your new NAND_ONFI (nand.h); both are 0x00001000. I know
> *some* options are written into nand_chip->options and later copied onto
> the options in nand_bbt_descr->options for BBT usage, e.g., I just rewrote
> part of this as a new function nand_create_default_bbt_descr() in
> nand_bbt.c:
> http://lists.infradead.org/pipermail/linux-mtd/2010-July/030911.html
> 
> Other pieces of the code perform similar functions at the moment.

I admit I did not look too closely at these recent changes, but it seems safe 
to move the NAND_ONFI a bit higher. I will take the other comments and respin 
the patch. Thanks!
--
Florian
Florian Fainelli - July 29, 2010, 8:51 a.m.
Hi Matthieu,

On Thursday 29 July 2010 09:54:20 Matthieu CASTET wrote:
> Hi,
> 
> Florian Fainelli a écrit :
> > A nand_chip which has valid ONFI parameters gets its options field
> > updated with the NAND_ONFI flag. In that case both the ONFI version (in
> > BCD format) as well as the complete page parameters is available in the
> > struct nand_chip. This allows for better detection of some new devices,
> > as well as fine tuning of NAND driver timings. This patch only adds
> > support for ONFI 1.0 parameters.
> > 
> > Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
> > Signed-off-by: Florian Fainelli <ffainelli@freebox.fr>
> > ---
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 4a7b864..c255cec 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -2773,6 +2773,83 @@ static void nand_set_defaults(struct nand_chip
> > *chip, int busw)
> > 
> >  }
> > 
> > +
> > +/*
> > + * Check whether flash support ONFI and read ONFI parameters in that
> > + * case
> > + */
> > +static void nand_read_onfi(struct mtd_info *mtd, struct nand_chip *chip)
> > +{
> > +	struct nand_onfi_params *p;
> > +	uint8_t sig[4];
> > +	uint16_t val;
> > +
> > +	if (!chip->cmdfunc || !chip->read_buf)
> > +		return;
> > +
> > +	/* read ONFI signature */
> > +	chip->cmdfunc(mtd, NAND_CMD_READID, NAND_ADDR_ONFI_ID, -1);
> > +	chip->read_buf(mtd, sig, sizeof(sig));
> > +
> > +	if (memcmp(sig, "ONFI", sizeof(sig)))
> > +		return;
> > +
> > +	/* ONFI seems supported */
> > +	chip->cmdfunc(mtd, NAND_CMD_READ_ONFI_PARAMS, 0, -1);
> > +	p = &chip->onfi_params;
> > +	chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
> > +
> > +	/* recheck signature */
> > +	if (memcmp(p->sig, "ONFI", sizeof(p->sig))) {
> > +		printk(KERN_INFO "%s: bad ONFI params signature\n", __func__);
> > +		return;
> > +	}
> 
> You need to check crc. onfi param can be corrupted (bitflip ?) and
> should try at least 3 page.

Ok.

> 
> 
> Also you don't handle endianness (integer are little endian) for value
> in nand_onfi_params.

Yes, so far the drivers using those values were doing the correct endian 
conversion when they need to use them.

> 
> Also I am not sure it is worth to export all the onfi param. Fill the
> mtd param for page, erase, oob size and other stuff.

What we were interested about are mostly timings, manufacturer and model 
string because they can help handling new parts correctly.

> After all rev info and features block, manufacturer information block,
> memory organization block should only be used by mtd layer.

You are right.

> Only electrical parameter block can interest driver for fine tunning

Yes.

> 
> >   * Get the flash and manufacturer id and lookup if the type is supported
> >   */
> >  
> >  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> > 
> > @@ -2958,10 +3035,19 @@ static struct nand_flash_dev
> > *nand_get_flash_type(struct mtd_info *mtd,
> > 
> >  	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> >  	
> >  		chip->cmdfunc = nand_command_lp;
> > 
> > +	nand_read_onfi(mtd, chip);
> > +
> > 
> >  	printk(KERN_INFO "NAND device: Manufacturer ID:"
> >  	
> >  	       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, dev_id,
> >  	       nand_manuf_ids[maf_idx].name, type->name);
> > 
> > +	if (chip->options & NAND_ONFI)
> > +		printk(KERN_INFO "NAND is ONFI %d.%d compliant "
> > +		       "(man:%s model:%s)\n",
> > +		       chip->onfi_version / 10, chip->onfi_version % 10,
> > +		       chip->onfi_params.manufacturer,
> > +		       chip->onfi_params.model);
> > +
> > 
> >  	return type;
> >  
> >  }
> 
> This won't work this unknown nand, and not work with some LP nand that
> doesn't provide additional id bytes.

So how do you see things regarding the provisioning of the relevant ONFI 
parameters?
--
Florian
Artem Bityutskiy - Aug. 5, 2010, 4:54 a.m.
On Thu, 2010-07-29 at 00:47 +0200, Florian Fainelli wrote:
> A nand_chip which has valid ONFI parameters gets its options field updated
> with the NAND_ONFI flag. In that case both the ONFI version (in BCD format)
> as well as the complete page parameters is available in the struct nand_chip.
> This allows for better detection of some new devices, as well as fine tuning of
> NAND driver timings. This patch only adds support for ONFI 1.0 parameters.
> 
> Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
> Signed-off-by: Florian Fainelli <ffainelli@freebox.fr>

I assume you will send v2 of this patch, so not taking this one.
Maxime Bizon - Aug. 5, 2010, 12:56 p.m.
On Thu, 2010-08-05 at 07:54 +0300, Artem Bityutskiy wrote:
> On Thu, 2010-07-29 at 00:47 +0200, Florian Fainelli wrote:
> > A nand_chip which has valid ONFI parameters gets its options field updated
> > with the NAND_ONFI flag. In that case both the ONFI version (in BCD format)
> > as well as the complete page parameters is available in the struct nand_chip.
> > This allows for better detection of some new devices, as well as fine tuning of
> > NAND driver timings. This patch only adds support for ONFI 1.0 parameters.
> > 
> > Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
> > Signed-off-by: Florian Fainelli <ffainelli@freebox.fr>
> 
> I assume you will send v2 of this patch, so not taking this one.

Please don't

We still need to address all remarks Matthieu did, and of course gives
this more testing (this was more an "RFC" than a "PATCH", sorry)

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a7b864..c255cec 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2773,6 +2773,83 @@  static void nand_set_defaults(struct nand_chip *chip, int busw)
 }
 
 /*
+ * sanitize ONFI strings so we can safely print them
+ */
+static void sanitize_string(uint8_t *s, size_t len)
+{
+	ssize_t i;
+
+	/* null terminate */
+	s[len - 1] = 0;
+
+	/* remove non printable chars */
+	for (i = 0; i < len - 1; i++) {
+		if (s[i] < ' ' || s[i] > 127)
+			s[i] = '?';
+	}
+
+	/* remove trailing spaces */
+	for (i = len - 1; i >= 0; i--) {
+		if (s[i] && s[i] != ' ')
+			break;
+		s[i] = 0;
+	}
+}
+
+/*
+ * Check whether flash support ONFI and read ONFI parameters in that
+ * case
+ */
+static void nand_read_onfi(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	struct nand_onfi_params *p;
+	uint8_t sig[4];
+	uint16_t val;
+
+	if (!chip->cmdfunc || !chip->read_buf)
+		return;
+
+	/* read ONFI signature */
+	chip->cmdfunc(mtd, NAND_CMD_READID, NAND_ADDR_ONFI_ID, -1);
+	chip->read_buf(mtd, sig, sizeof(sig));
+
+	if (memcmp(sig, "ONFI", sizeof(sig)))
+		return;
+
+	/* ONFI seems supported */
+	chip->cmdfunc(mtd, NAND_CMD_READ_ONFI_PARAMS, 0, -1);
+	p = &chip->onfi_params;
+	chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+
+	/* recheck signature */
+	if (memcmp(p->sig, "ONFI", sizeof(p->sig))) {
+		printk(KERN_INFO "%s: bad ONFI params signature\n", __func__);
+		return;
+	}
+
+	/* check version */
+	val = le16_to_cpu(p->revision);
+	if (!is_power_of_2(val) || val == 1 || val > (1 << 4)) {
+		printk(KERN_INFO "%s: unsupported ONFI version\n", __func__);
+		return;
+	}
+
+	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;
+
+	chip->options |= NAND_ONFI;
+
+	sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+	sanitize_string(p->model, sizeof(p->model));
+}
+
+/*
  * Get the flash and manufacturer id and lookup if the type is supported
  */
 static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
@@ -2958,10 +3035,19 @@  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
 		chip->cmdfunc = nand_command_lp;
 
+	nand_read_onfi(mtd, chip);
+
 	printk(KERN_INFO "NAND device: Manufacturer ID:"
 	       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, dev_id,
 	       nand_manuf_ids[maf_idx].name, type->name);
 
+	if (chip->options & NAND_ONFI)
+		printk(KERN_INFO "NAND is ONFI %d.%d compliant "
+		       "(man:%s model:%s)\n",
+		       chip->onfi_version / 10, chip->onfi_version % 10,
+		       chip->onfi_params.manufacturer,
+		       chip->onfi_params.model);
+
 	return type;
 }
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index a81b185..ad7f58f 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -118,6 +118,10 @@  extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 #define NAND_CMD_STATUS_RESET	0x7f
 #define NAND_CMD_STATUS_CLEAR	0xff
 
+/* Extended commands for ONFI devices */
+#define NAND_CMD_READ_ONFI_PARAMS	0xEC
+#define NAND_ADDR_ONFI_ID	0x20
+
 #define NAND_CMD_NONE		-1
 
 /* Status bits */
@@ -190,6 +194,9 @@  typedef enum {
 /* Device behaves just like nand, but is readonly */
 #define NAND_ROM		0x00000800
 
+/* Chip supports ONFI */
+#define NAND_ONFI		0x00001000
+
 /* Options valid for Samsung large page devices */
 #define NAND_SAMSUNG_LP_OPTIONS \
 	(NAND_NO_PADDING | NAND_CACHEPRG | NAND_COPYBACK)
@@ -229,6 +236,61 @@  typedef enum {
 /* Keep gcc happy */
 struct nand_chip;
 
+struct nand_onfi_params {
+	/* rev info and features block */
+	uint8_t		sig[4]; /* 'O' 'N' 'F' 'I'  */
+	uint16_t	revision;
+	uint16_t	features;
+	uint16_t	opt_cmd;
+	uint8_t		reserved[22];
+
+	/* manufacturer information block */
+	char		manufacturer[12];
+	char		model[20];
+	uint8_t		jedec_id;
+	uint16_t	date_code;
+	uint8_t		reserved2[13];
+
+	/* memory organization block */
+	uint32_t	byte_per_page;
+	uint16_t	spare_bytes_per_page;
+	uint32_t	data_bytes_per_ppage;
+	uint16_t	sparre_bytes_per_ppage;
+	uint32_t	pages_per_block;
+	uint32_t	blocks_per_lun;
+	uint8_t		lun_count;
+	uint8_t		addr_cycles;
+	uint8_t		bits_per_cell;
+	uint16_t	bb_per_lun;
+	uint16_t	block_endurance;
+	uint8_t		guaranteed_good_blocks;
+	uint16_t	guaranteed_block_endurance;
+	uint8_t		programs_per_page;
+	uint8_t		ppage_attr;
+	uint8_t		ecc_bits;
+	uint8_t		interleaved_bits;
+	uint8_t		interleaved_ops;
+	uint8_t		reserved3[13];
+	uint8_t		io_pin_capacitance_max;
+
+	/* electrical parameter block */
+	uint16_t	async_timing_mode;
+	uint16_t	program_cache_timing_mode;
+	uint16_t	t_prog;
+	uint16_t	t_bers;
+	uint16_t	t_r;
+	uint16_t	t_ccs;
+	uint16_t	src_sync_timing_mode;
+	uint16_t	src_ssync_features;
+	uint16_t	clk_pin_capacitance_typ;
+	uint16_t	io_pin_capacitance_typ;
+	uint16_t	input_pin_capacitance_typ;
+	uint8_t		input_pin_capacitance_max;
+	uint8_t		driver_strenght_support;
+	uint16_t	t_int_r;
+	uint16_t	t_ald;
+} __attribute__((packed));
+
 /**
  * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
  * @lock:               protection lock
@@ -354,6 +416,8 @@  struct nand_buffers {
  * @chip_shift:		[INTERN] number of address bits in one chip
  * @options:		[BOARDSPECIFIC] various chip options. They can partly be set to inform nand_scan about
  *			special functionality. See the defines for further explanation
+ * @onfi_version:	Supported ONFI version (10 => ONFI 1.0)
+ * @onfi_params:	ONFI parameters
  * @badblockpos:	[INTERN] position of the bad block marker in the oob area
  * @cellinfo:		[INTERN] MLC/multichip data from chip ident
  * @numchips:		[INTERN] number of physical chips
@@ -413,6 +477,9 @@  struct nand_chip {
 	int		badblockpos;
 	int		badblockbits;
 
+	int		onfi_version;
+	struct nand_onfi_params	onfi_params;
+
 	flstate_t	state;
 
 	uint8_t		*oob_poi;