public inbox for linux-mtd@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH] NAND: add support for reading ONFI parameters from NAND device
@ 2010-07-28 22:47 Florian Fainelli
  2010-07-28 23:38 ` Brian Norris
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Florian Fainelli @ 2010-07-28 22:47 UTC (permalink / raw)
  To: linux-mtd@lists.infradead.org, David Woodhouse, Brian Norris,
	Matthieu CASTET, Maxime Bizon

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)
 }
 
 /*
+ * 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;

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-07-28 22:47 [PATCH] NAND: add support for reading ONFI parameters from NAND device Florian Fainelli
@ 2010-07-28 23:38 ` Brian Norris
  2010-07-29  8:10   ` Florian Fainelli
  2010-07-29  7:54 ` Matthieu CASTET
  2010-08-05  4:54 ` Artem Bityutskiy
  2 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2010-07-28 23:38 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: David Woodhouse, linux-mtd@lists.infradead.org, Matthieu CASTET,
	Maxime Bizon

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-07-28 22:47 [PATCH] NAND: add support for reading ONFI parameters from NAND device Florian Fainelli
  2010-07-28 23:38 ` Brian Norris
@ 2010-07-29  7:54 ` Matthieu CASTET
  2010-07-29  8:51   ` Florian Fainelli
  2010-08-05  4:54 ` Artem Bityutskiy
  2 siblings, 1 reply; 11+ messages in thread
From: Matthieu CASTET @ 2010-07-29  7:54 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: David Woodhouse, linux-mtd@lists.infradead.org, Brian Norris,
	Maxime Bizon

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-07-28 23:38 ` Brian Norris
@ 2010-07-29  8:10   ` Florian Fainelli
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Fainelli @ 2010-07-29  8:10 UTC (permalink / raw)
  To: linux-mtd; +Cc: Maxime Bizon, David Woodhouse, Matthieu CASTET, Brian Norris

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-07-29  7:54 ` Matthieu CASTET
@ 2010-07-29  8:51   ` Florian Fainelli
  2010-08-02  9:25     ` Matthieu CASTET
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2010-07-29  8:51 UTC (permalink / raw)
  To: linux-mtd; +Cc: Maxime Bizon, David Woodhouse, Brian Norris, Matthieu CASTET

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-07-29  8:51   ` Florian Fainelli
@ 2010-08-02  9:25     ` Matthieu CASTET
  2010-08-02 11:55       ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Matthieu CASTET @ 2010-08-02  9:25 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: David Woodhouse, linux-mtd@lists.infradead.org, Brian Norris,
	Maxime Bizon

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

Florian Fainelli a écrit :
> Hi Matthieu,
> 
> On Thursday 29 July 2010 09:54:20 Matthieu CASTET wrote:
>> Hi,
>>
>>
>> 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.
In that case use le16, le32, ... type. Also prefer kernel type over 
uintx_t 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?
I will see something like in the patch attached in
http://article.gmane.org/gmane.linux.drivers.mtd/30935.

ONFI parsing is done early in nand_get_flash_type (unknow chip or LP nand).
If the ONFI parsing is ok we bypass the old identification method
(additional id bytes).

As an example I attach a patch that mix your patch and mine.


Matthieu

PS : the NAND_ONFI flags seems useless, we can use onfi_version (0 means 
no onfi).


[-- Attachment #2: onfi.diff --]
[-- Type: text/x-diff, Size: 10918 bytes --]

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a7b864..25a44fa 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2772,15 +2772,50 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 
 }
 
+static u16 onfi_crc(u16 crc, unsigned char const *p, size_t len)
+{
+	int i;
+	while (len--) {
+		crc ^= *p++ << 8;
+		for (i = 0; i < 8; i++)
+			crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
+	}
+	return crc;
+}
+
+/*
+ * 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;
+	}
+}
+
 /*
  * 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,
 						  struct nand_chip *chip,
-						  int busw, int *maf_id,
+						  int busw, int *maf_id, int *dev_id
 						  struct nand_flash_dev *type)
 {
-	int i, dev_id, maf_idx;
+	int i, maf_idx;
 	u8 id_data[8];
 
 	/* Select the device */
@@ -2797,7 +2832,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
 	/* Read manufacturer and device IDs */
 	*maf_id = chip->read_byte(mtd);
-	dev_id = chip->read_byte(mtd);
+	*dev_id = chip->read_byte(mtd);
 
 	/* Try again to make sure, as some systems the bus-hold or other
 	 * interface concerns can cause random data which looks like a
@@ -2807,15 +2842,13 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
 	chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 
-	/* Read entire ID string */
-
-	for (i = 0; i < 8; i++)
+	for (i = 0; i < 2; i++)
 		id_data[i] = chip->read_byte(mtd);
 
-	if (id_data[0] != *maf_id || id_data[1] != dev_id) {
+	if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
 		printk(KERN_INFO "%s: second ID read did not match "
 		       "%02x,%02x against %02x,%02x\n", __func__,
-		       *maf_id, dev_id, id_data[0], id_data[1]);
+		       *maf_id, *dev_id, id_data[0], id_data[1]);
 		return ERR_PTR(-ENODEV);
 	}
 
@@ -2823,8 +2856,79 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		type = nand_flash_ids;
 
 	for (; type->name != NULL; type++)
-		if (dev_id == type->id)
+		if (*dev_id == type->id)
                         break;
+#if 1
+	chip->onfi_version = 0;
+	if (!type->name || !type->pagesize) {
+		/* try ONFI for unknow chip or LP */
+		chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
+		if (chip->read_byte(mtd) == 'O' &&
+			chip->read_byte(mtd) == 'N' &&
+			chip->read_byte(mtd) == 'F' &&
+			chip->read_byte(mtd) == 'I') {
+
+			struct nand_onfi_params *p = &chip->onfi_params;
+			int i;
+
+			printk(KERN_INFO "ONFI flash detected\n");
+			chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+			for (i = 0; i < 3; i++) {
+				/* XXX this that ok to use read_buf at this stage ? */
+				chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+				if (onfi_crc(0x4F4E, (uint8_t *)p, 254) == le16_to_cpu(p->crc))
+				{
+					 printk(KERN_INFO "ONFI param page %d valid\n", i);
+					 break;
+				}
+			}
+			if (i < 3) {
+				/* check version */
+				int 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__);
+				}
+				else {
+					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;
+				}
+			}
+			if (chip->onfi_version) {
+				sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+				sanitize_string(p->model, sizeof(p->model));
+				if (!mtd->name)
+					mtd->name = p->model;
+				mtd->writesize = le32_to_cpu(p->byte_per_page);
+				mtd->erasesize = le32_to_cpu(p->pages_per_block)*mtd->writesize;
+				mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+				chip->chipsize = le32_to_cpu(p->blocks_per_lun) * mtd->erasesize;
+				busw = 0;
+				if (le16_to_cpu(p->features) & 1)
+					busw = NAND_BUSWIDTH_16;
+
+				chip->options &= ~NAND_CHIPOPTIONS_MSK;
+				chip->options |= (NAND_NO_READRDY | NAND_NO_AUTOINCR) & NAND_CHIPOPTIONS_MSK;
+
+				/* emulate cellinfo ??? */
+				//chip->cellinfo = 0x8;
+				goto ident_done;
+
+			}
+		}
+	}
+#endif
+	chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
+
+	/* Read entire ID string */
+
+	for (i = 0; i < 8; i++)
+		id_data[i] = chip->read_byte(mtd);
 
 	if (!type->name)
 		return ERR_PTR(-ENODEV);
@@ -2886,6 +2990,21 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		mtd->oobsize = mtd->writesize / 32;
 		busw = type->options & NAND_BUSWIDTH_16;
 	}
+	/* Get chip options, preserve non chip based options */
+	chip->options &= ~NAND_CHIPOPTIONS_MSK;
+	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
+
+	/* Check if chip is a not a samsung device. Do not clear the
+	 * options for chips which are not having an extended id.
+	 */
+	if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
+		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
+ident_done:
+
+	/*
+	 * Set chip as a default. Board drivers can override it, if necessary
+	 */
+	chip->options |= NAND_NO_AUTOINCR;
 
 	/* Try to identify manufacturer */
 	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
@@ -2900,7 +3019,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
 		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, mtd->name);
+		       *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
 		printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
 		       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
 		       busw ? 16 : 8);
@@ -2924,21 +3043,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		NAND_LARGE_BADBLOCK_POS : NAND_SMALL_BADBLOCK_POS;
 	chip->badblockbits = 8;
 
-	/* Get chip options, preserve non chip based options */
-	chip->options &= ~NAND_CHIPOPTIONS_MSK;
-	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
-
-	/*
-	 * Set chip as a default. Board drivers can override it, if necessary
-	 */
-	chip->options |= NAND_NO_AUTOINCR;
-
-	/* Check if chip is a not a samsung device. Do not clear the
-	 * options for chips which are not having an extended id.
-	 */
-	if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
-		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
-
 	/*
 	 * Bad block marker is stored in the last page of each block
 	 * on Samsung and Hynix MLC devices
@@ -2958,9 +3062,10 @@ 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;
 
+	/* TODO onfi flash name */
 	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);
+	       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
+	       nand_manuf_ids[maf_idx].name, chip->onfi_version?type->name:chip->onfi_params.model);
 
 	return type;
 }
@@ -2979,7 +3084,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		    struct nand_flash_dev *table)
 {
-	int i, busw, nand_maf_id;
+	int i, busw, nand_maf_id, nand_dev_id;
 	struct nand_chip *chip = mtd->priv;
 	struct nand_flash_dev *type;
 
@@ -2989,7 +3094,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	nand_set_defaults(chip, busw);
 
 	/* Read the flash type */
-	type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, table);
+	type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, &nand_dev_id, table);
 
 	if (IS_ERR(type)) {
 		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
@@ -3007,7 +3112,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 		/* Read manufacturer and device IDs */
 		if (nand_maf_id != chip->read_byte(mtd) ||
-		    type->id != chip->read_byte(mtd))
+		    nand_dev_id != chip->read_byte(mtd))
 			break;
 	}
 	if (i > 1)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index a81b185..c358100 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -88,6 +88,7 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 #define NAND_CMD_RNDIN		0x85
 #define NAND_CMD_READID		0x90
 #define NAND_CMD_ERASE2		0xd0
+#define NAND_CMD_PARAM		0xec
 #define NAND_CMD_RESET		0xff
 
 #define NAND_CMD_LOCK		0x2a
@@ -229,6 +230,68 @@ typedef enum {
 /* Keep gcc happy */
 struct nand_chip;
 
+/* XXX use le16, le32 type */
+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];
+
+	/* electrical parameter block */
+	uint8_t		io_pin_capacitance_max;
+	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;
+	uint8_t		reserved4[7];
+
+	/* vendor */
+	uint8_t		reserved5[90];
+
+	__le16 crc;
+} __attribute__((packed));
+
 /**
  * struct nand_hw_control - Control structure for hardware controller (e.g ECC generator) shared among independent devices
  * @lock:               protection lock
@@ -413,6 +476,9 @@ struct nand_chip {
 	int		badblockpos;
 	int		badblockbits;
 
+	int		onfi_version;
+	struct nand_onfi_params	onfi_params;
+
 	flstate_t	state;
 
 	uint8_t		*oob_poi;

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-08-02  9:25     ` Matthieu CASTET
@ 2010-08-02 11:55       ` Florian Fainelli
  2010-08-09  9:25         ` Matthieu CASTET
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2010-08-02 11:55 UTC (permalink / raw)
  To: linux-mtd; +Cc: Maxime Bizon, David Woodhouse, Brian Norris, Matthieu CASTET

Hi Matthieu,

On Monday 02 August 2010 11:25:49 Matthieu CASTET wrote:
> Florian Fainelli a écrit :
> > Hi Matthieu,
> > 
> > On Thursday 29 July 2010 09:54:20 Matthieu CASTET wrote:
> >> Hi,
> >> 
> >> 
> >> 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.
> 
> In that case use le16, le32, ... type. Also prefer kernel type over
> uintx_t 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?
> 
> I will see something like in the patch attached in
> http://article.gmane.org/gmane.linux.drivers.mtd/30935.
> 
> ONFI parsing is done early in nand_get_flash_type (unknow chip or LP nand).
> If the ONFI parsing is ok we bypass the old identification method
> (additional id bytes).

Looks ok to me.

> 
> As an example I attach a patch that mix your patch and mine.
> 
> 
> Matthieu
> 
> PS : the NAND_ONFI flags seems useless, we can use onfi_version (0 means
> no onfi).

Right, thanks for noticing that.

I got a couple of comments on your patch that I inlined, the rest looks
good.
--
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a7b864..25a44fa 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2772,15 +2772,50 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 
 }
 
+static u16 onfi_crc(u16 crc, unsigned char const *p, size_t len)
+{
+       int i;
+       while (len--) {
+               crc ^= *p++ << 8;
+               for (i = 0; i < 8; i++)
+                       crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
+       }
+       return crc;
+}
+
+/*
+ * 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;
+       }
+}
+
 /*
  * 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,
                                                  struct nand_chip *chip,
-                                                 int busw, int *maf_id,
+                                                 int busw, int *maf_id, int *dev_id
                                                  struct nand_flash_dev *type)
 {
-       int i, dev_id, maf_idx;
+       int i, maf_idx;
        u8 id_data[8];
 
        /* Select the device */
@@ -2797,7 +2832,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
        /* Read manufacturer and device IDs */
        *maf_id = chip->read_byte(mtd);
-       dev_id = chip->read_byte(mtd);
+       *dev_id = chip->read_byte(mtd);
 
        /* Try again to make sure, as some systems the bus-hold or other
         * interface concerns can cause random data which looks like a
@@ -2807,15 +2842,13 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 
        chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 
-       /* Read entire ID string */
-
-       for (i = 0; i < 8; i++)
+       for (i = 0; i < 2; i++)
                id_data[i] = chip->read_byte(mtd);
 
-       if (id_data[0] != *maf_id || id_data[1] != dev_id) {
+       if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
                printk(KERN_INFO "%s: second ID read did not match "
                       "%02x,%02x against %02x,%02x\n", __func__,
-                      *maf_id, dev_id, id_data[0], id_data[1]);
+                      *maf_id, *dev_id, id_data[0], id_data[1]);
                return ERR_PTR(-ENODEV);
        }
 
@@ -2823,8 +2856,79 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                type = nand_flash_ids;
 
        for (; type->name != NULL; type++)
-               if (dev_id == type->id)
+               if (*dev_id == type->id)
                         break;
+#if 1
+       chip->onfi_version = 0;
+       if (!type->name || !type->pagesize) {
+               /* try ONFI for unknow chip or LP */
+               chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
+               if (chip->read_byte(mtd) == 'O' &&
+                       chip->read_byte(mtd) == 'N' &&
+                       chip->read_byte(mtd) == 'F' &&
+                       chip->read_byte(mtd) == 'I') {

Why not use what was in our original patch and do the memcmp? That looks
cleaner to me and allows to invert the logic on the if statement to get the
code cleaner. That's just cosmetic anyway.

+
+                       struct nand_onfi_params *p = &chip->onfi_params;
+                       int i;
+
+                       printk(KERN_INFO "ONFI flash detected\n");
+                       chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
+                       for (i = 0; i < 3; i++) {
+                               /* XXX this that ok to use read_buf at this stage ? */
+                               chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+                               if (onfi_crc(0x4F4E, (uint8_t *)p, 254) == le16_to_cpu(p->crc))
+                               {
+                                        printk(KERN_INFO "ONFI param page %d valid\n", i);
+                                        break;
+                               }
+                       }
+                       if (i < 3) {
+                               /* check version */
+                               int val = le16_to_cpu(p->revision);
+                               if (!is_power_of_2(val) || val == 1 || val > (1 << 4)) {

the !is_power_of_2 check does not work for ONFI version 2.1 (3), so I would only
keep the two other checks.

+                                       printk(KERN_INFO "%s: unsupported ONFI version\n", __func__);
+                               }
+                               else {
+                                       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;
+                               }
+                       }
+                       if (chip->onfi_version) {
+                               sanitize_string(p->manufacturer, sizeof(p->manufacturer));
+                               sanitize_string(p->model, sizeof(p->model));
+                               if (!mtd->name)
+                                       mtd->name = p->model;
+                               mtd->writesize = le32_to_cpu(p->byte_per_page);
+                               mtd->erasesize = le32_to_cpu(p->pages_per_block)*mtd->writesize;
+                               mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
+                               chip->chipsize = le32_to_cpu(p->blocks_per_lun) * mtd->erasesize;
+                               busw = 0;
+                               if (le16_to_cpu(p->features) & 1)
+                                       busw = NAND_BUSWIDTH_16;
+
+                               chip->options &= ~NAND_CHIPOPTIONS_MSK;
+                               chip->options |= (NAND_NO_READRDY | NAND_NO_AUTOINCR) & NAND_CHIPOPTIONS_MSK;
+
+                               /* emulate cellinfo ??? */
+                               //chip->cellinfo = 0x8;
+                               goto ident_done;
+
+                       }
+               }
+       }
+#endif
+       chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
+
+       /* Read entire ID string */
+
+       for (i = 0; i < 8; i++)
+               id_data[i] = chip->read_byte(mtd);
 
        if (!type->name)
                return ERR_PTR(-ENODEV);
@@ -2886,6 +2990,21 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                mtd->oobsize = mtd->writesize / 32;
                busw = type->options & NAND_BUSWIDTH_16;
        }
+       /* Get chip options, preserve non chip based options */
+       chip->options &= ~NAND_CHIPOPTIONS_MSK;
+       chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
+
+       /* Check if chip is a not a samsung device. Do not clear the
+        * options for chips which are not having an extended id.
+        */
+       if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
+               chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
+ident_done:
+
+       /*
+        * Set chip as a default. Board drivers can override it, if necessary
+        */
+       chip->options |= NAND_NO_AUTOINCR;
 
        /* Try to identify manufacturer */
        for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
@@ -2900,7 +3019,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
        if (busw != (chip->options & NAND_BUSWIDTH_16)) {
                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, mtd->name);
+                      *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
                printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
                       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
                       busw ? 16 : 8);
@@ -2924,21 +3043,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
                NAND_LARGE_BADBLOCK_POS : NAND_SMALL_BADBLOCK_POS;
        chip->badblockbits = 8;
 
-       /* Get chip options, preserve non chip based options */
-       chip->options &= ~NAND_CHIPOPTIONS_MSK;
-       chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
-
-       /*
-        * Set chip as a default. Board drivers can override it, if necessary
-        */
-       chip->options |= NAND_NO_AUTOINCR;
-
-       /* Check if chip is a not a samsung device. Do not clear the
-        * options for chips which are not having an extended id.
-        */
-       if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
-               chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
-
        /*
         * Bad block marker is stored in the last page of each block
         * on Samsung and Hynix MLC devices
@@ -2958,9 +3062,10 @@ 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;
 
+       /* TODO onfi flash name */
        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);
+              " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
+              nand_manuf_ids[maf_idx].name, chip->onfi_version?type->name:chip->onfi_params.model);
 
        return type;
 }
@@ -2979,7 +3084,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 int nand_scan_ident(struct mtd_info *mtd, int maxchips,
                    struct nand_flash_dev *table)
 {
-       int i, busw, nand_maf_id;
+       int i, busw, nand_maf_id, nand_dev_id;
        struct nand_chip *chip = mtd->priv;
        struct nand_flash_dev *type;
 
@@ -2989,7 +3094,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
        nand_set_defaults(chip, busw);
 
        /* Read the flash type */
-       type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, table);
+       type = nand_get_flash_type(mtd, chip, busw, &nand_maf_id, &nand_dev_id, table);
 
        if (IS_ERR(type)) {
                if (!(chip->options & NAND_SCAN_SILENT_NODEV))
@@ -3007,7 +3112,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
                chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
                /* Read manufacturer and device IDs */
                if (nand_maf_id != chip->read_byte(mtd) ||
-                   type->id != chip->read_byte(mtd))
+                   nand_dev_id != chip->read_byte(mtd))
                        break;
        }
        if (i > 1)

--
Florian

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-07-28 22:47 [PATCH] NAND: add support for reading ONFI parameters from NAND device Florian Fainelli
  2010-07-28 23:38 ` Brian Norris
  2010-07-29  7:54 ` Matthieu CASTET
@ 2010-08-05  4:54 ` Artem Bityutskiy
  2010-08-05 12:56   ` Maxime Bizon
  2 siblings, 1 reply; 11+ messages in thread
From: Artem Bityutskiy @ 2010-08-05  4:54 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: David Woodhouse, linux-mtd@lists.infradead.org, Matthieu CASTET,
	Brian Norris, Maxime Bizon

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.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-08-05  4:54 ` Artem Bityutskiy
@ 2010-08-05 12:56   ` Maxime Bizon
  0 siblings, 0 replies; 11+ messages in thread
From: Maxime Bizon @ 2010-08-05 12:56 UTC (permalink / raw)
  To: dedekind1
  Cc: David Woodhouse, linux-mtd@lists.infradead.org, Matthieu CASTET,
	Brian Norris, Florian Fainelli


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)

-- 
Maxime

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-08-02 11:55       ` Florian Fainelli
@ 2010-08-09  9:25         ` Matthieu CASTET
  2010-08-09  9:43           ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Matthieu CASTET @ 2010-08-09  9:25 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: David Woodhouse, linux-mtd@lists.infradead.org, Brian Norris,
	Maxime Bizon

Hi Florian,

Florian Fainelli a écrit :
> Hi Matthieu,
> 
> On Monday 02 August 2010 11:25:49 Matthieu CASTET wrote:
>> Florian Fainelli a écrit :
>>> Hi Matthieu,
>>>
>>> On Thursday 29 July 2010 09:54:20 Matthieu CASTET wrote:
>>>> Hi,
>>>>
>>>>
>>>> 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.
>> In that case use le16, le32, ... type. Also prefer kernel type over
>> uintx_t 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?
>> I will see something like in the patch attached in
>> http://article.gmane.org/gmane.linux.drivers.mtd/30935.
>>
>> ONFI parsing is done early in nand_get_flash_type (unknow chip or LP nand).
>> If the ONFI parsing is ok we bypass the old identification method
>> (additional id bytes).
> 
> Looks ok to me.
> 
>> As an example I attach a patch that mix your patch and mine.
>>
>>
>> Matthieu
>>
>> PS : the NAND_ONFI flags seems useless, we can use onfi_version (0 means
>> no onfi).
> 
> Right, thanks for noticing that.
> 
> I got a couple of comments on your patch that I inlined, the rest looks
> good.
> --

> +#if 1
> +       chip->onfi_version = 0;
> +       if (!type->name || !type->pagesize) {
> +               /* try ONFI for unknow chip or LP */
> +               chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> +               if (chip->read_byte(mtd) == 'O' &&
> +                       chip->read_byte(mtd) == 'N' &&
> +                       chip->read_byte(mtd) == 'F' &&
> +                       chip->read_byte(mtd) == 'I') {
> 
> Why not use what was in our original patch and do the memcmp? That looks
> cleaner to me and allows to invert the logic on the if statement to get the
> code cleaner. That's just cosmetic anyway.
I wanted to avoid to use read_buf, because some advanced controller 
(those who implement cmdfunc) need to overrides all io access.
But some driver assumed  that nand_scan_ident only used read_byte. That 
the case of the denali driver [1]. Using it will cause random read in 
memory and likely a kernel panic.

But we need read_buf for reading onfi page. Also these advanced 
controllers will break because they won't handle correctly in cmdfunc 
new NAND_CMD_READID and NAND_CMD_PARAM.

I don't know what the best way to handle them.


> +                       if (i < 3) {
> +                               /* check version */
> +                               int val = le16_to_cpu(p->revision);
> +                               if (!is_power_of_2(val) || val == 1 || val > (1 << 4)) {
> 
> the !is_power_of_2 check does not work for ONFI version 2.1 (3), so I would only
> keep the two other checks.
> 
Ok.

Will you take care to post a new patch ?


Matthieu


[1]
/* register the driver with the NAND core subsystem */
     denali->nand.select_chip = denali_select_chip;
     denali->nand.cmdfunc = denali_cmdfunc;
     denali->nand.read_byte = denali_read_byte;
     denali->nand.waitfunc = denali_waitfunc;

     /* scan for NAND devices attached to the controller
      * this is the first stage in a two step process to register
      * with the nand subsystem */
     if (nand_scan_ident(&denali->mtd, LLD_MAX_FLASH_BANKS, NULL))

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] NAND: add support for reading ONFI parameters from NAND device
  2010-08-09  9:25         ` Matthieu CASTET
@ 2010-08-09  9:43           ` Florian Fainelli
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Fainelli @ 2010-08-09  9:43 UTC (permalink / raw)
  To: linux-mtd; +Cc: Maxime Bizon, David Woodhouse, Brian Norris, Matthieu CASTET

Hi Matthieu,

On Monday 09 August 2010 11:25:18 Matthieu CASTET wrote:
> Hi Florian,
> 
> Florian Fainelli a écrit :
> > Hi Matthieu,
> > 
> > On Monday 02 August 2010 11:25:49 Matthieu CASTET wrote:
> >> Florian Fainelli a écrit :
> >>> Hi Matthieu,
> >>> 
> >>> On Thursday 29 July 2010 09:54:20 Matthieu CASTET wrote:
> >>>> Hi,
> >>>> 
> >>>> 
> >>>> 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.
> >> 
> >> In that case use le16, le32, ... type. Also prefer kernel type over
> >> uintx_t 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?
> >> 
> >> I will see something like in the patch attached in
> >> http://article.gmane.org/gmane.linux.drivers.mtd/30935.
> >> 
> >> ONFI parsing is done early in nand_get_flash_type (unknow chip or LP
> >> nand). If the ONFI parsing is ok we bypass the old identification
> >> method (additional id bytes).
> > 
> > Looks ok to me.
> > 
> >> As an example I attach a patch that mix your patch and mine.
> >> 
> >> 
> >> Matthieu
> >> 
> >> PS : the NAND_ONFI flags seems useless, we can use onfi_version (0 means
> >> no onfi).
> > 
> > Right, thanks for noticing that.
> > 
> > I got a couple of comments on your patch that I inlined, the rest looks
> > good.
> > --
> > 
> > +#if 1
> > +       chip->onfi_version = 0;
> > +       if (!type->name || !type->pagesize) {
> > +               /* try ONFI for unknow chip or LP */
> > +               chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> > +               if (chip->read_byte(mtd) == 'O' &&
> > +                       chip->read_byte(mtd) == 'N' &&
> > +                       chip->read_byte(mtd) == 'F' &&
> > +                       chip->read_byte(mtd) == 'I') {
> > 
> > Why not use what was in our original patch and do the memcmp? That looks
> > cleaner to me and allows to invert the logic on the if statement to get
> > the code cleaner. That's just cosmetic anyway.
> 
> I wanted to avoid to use read_buf, because some advanced controller
> (those who implement cmdfunc) need to overrides all io access.

Ok.

> But some driver assumed  that nand_scan_ident only used read_byte. That
> the case of the denali driver [1]. Using it will cause random read in
> memory and likely a kernel panic.

Ok, then I will update it as part as the patch adding ONFI reading so that it 
is future-proof anyway.

> 
> But we need read_buf for reading onfi page. Also these advanced
> controllers will break because they won't handle correctly in cmdfunc
> new NAND_CMD_READID and NAND_CMD_PARAM.
> 
> I don't know what the best way to handle them.

Such driver need to handle those anyway.

> 
> > +                       if (i < 3) {
> > +                               /* check version */
> > +                               int val = le16_to_cpu(p->revision);
> > +                               if (!is_power_of_2(val) || val == 1 ||
> > val > (1 << 4)) {
> > 
> > the !is_power_of_2 check does not work for ONFI version 2.1 (3), so I
> > would only keep the two other checks.
> 
> Ok.
> 
> Will you take care to post a new patch ?

Sure, thanks for your follow-up.

> 
> 
> Matthieu
> 
> 
> [1]
> /* register the driver with the NAND core subsystem */
>      denali->nand.select_chip = denali_select_chip;
>      denali->nand.cmdfunc = denali_cmdfunc;
>      denali->nand.read_byte = denali_read_byte;
>      denali->nand.waitfunc = denali_waitfunc;
> 
>      /* scan for NAND devices attached to the controller
>       * this is the first stage in a two step process to register
>       * with the nand subsystem */
>      if (nand_scan_ident(&denali->mtd, LLD_MAX_FLASH_BANKS, NULL))
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2010-08-09  9:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-07-28 22:47 [PATCH] NAND: add support for reading ONFI parameters from NAND device Florian Fainelli
2010-07-28 23:38 ` Brian Norris
2010-07-29  8:10   ` Florian Fainelli
2010-07-29  7:54 ` Matthieu CASTET
2010-07-29  8:51   ` Florian Fainelli
2010-08-02  9:25     ` Matthieu CASTET
2010-08-02 11:55       ` Florian Fainelli
2010-08-09  9:25         ` Matthieu CASTET
2010-08-09  9:43           ` Florian Fainelli
2010-08-05  4:54 ` Artem Bityutskiy
2010-08-05 12:56   ` Maxime Bizon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox