Patchwork Check flag status register for Micron n25q512a

login
register
mail settings
Submitter Insop Song
Date Jan. 6, 2014, 5:21 a.m.
Message ID <6bf927e513554e628fe15d309aac698e@BLUPR07MB002.namprd07.prod.outlook.com>
Download mbox | patch
Permalink /patch/307106/
State Superseded
Headers show

Comments

Insop Song - Jan. 6, 2014, 5:21 a.m.
All,

In order to use Micron n25q512a, MTD, two changes are required as follows:
- jedec code should be fixed
- flag status should be read for writing.

Check flag status register for Micron n25q512a
    - Programing Micron n25q512a requires to check flag status register
    - According to datasheet
    	"
    	The flag status register must be read any time a PROGRAM, ERASE,
    	SUSPEND/RESUME command is issued, or after a RESET command while device
    	is busy. The cycle is not complete until bit 7 of the flag status
    	register output 1.
    	"
    - Ref: https://www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf




Signed-off-by: Insop Song <insop.song@gainspeed.com>
---
 drivers/mtd/devices/m25p80.c |   91 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)
Brian Norris - Feb. 27, 2014, 7:33 a.m.
+ Marek

Hi Insop, 

On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
> In order to use Micron n25q512a, MTD, two changes are required as follows:
> - jedec code should be fixed

I have a feeling there are more than one "n25q512a" device, with
different IDs.

> - flag status should be read for writing.
> 
> Check flag status register for Micron n25q512a
>     - Programing Micron n25q512a requires to check flag status register
>     - According to datasheet
>     	"
>     	The flag status register must be read any time a PROGRAM, ERASE,
>     	SUSPEND/RESUME command is issued, or after a RESET command while device
>     	is busy. The cycle is not complete until bit 7 of the flag status
>     	register output 1.
>     	"
>     - Ref: https://www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf

Hmm, are you sure that all Micron n25q512a need to check the flag status
register? I'll check my datasheets when I'm back in the office, but this
seems peculiar.
 
> Signed-off-by: Insop Song <insop.song@gainspeed.com>
> ---
>  drivers/mtd/devices/m25p80.c |   91 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..e53e522 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -38,6 +38,7 @@
>  /* Flash opcodes. */
>  #define	OPCODE_WREN		0x06	/* Write enable */
>  #define	OPCODE_RDSR		0x05	/* Read status register */
> +#define	OPCODE_RDFSR		0x70	/* Read flag status register */
>  #define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
>  #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
>  #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
> @@ -70,6 +71,8 @@
>  /* Status Register bits. */
>  #define	SR_WIP			1	/* Write in progress */
>  #define	SR_WEL			2	/* Write enable latch */
> +/* Flag Status Register bits. */
> +#define	FSR_RDY			0x80	/* FSR ready */
>  /* meaning of other SR_* bits may differ between vendors */
>  #define	SR_BP0			4	/* Block protect 0 */
>  #define	SR_BP1			8	/* Block protect 1 */
> @@ -95,6 +98,7 @@ struct m25p {
>  	u8			program_opcode;
>  	u8			*command;
>  	bool			fast_read;
> +	bool			flag_status;
>  };
>  
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -131,6 +135,28 @@ static int read_sr(struct m25p *flash)
>  }
>  
>  /*
> + * Read the flag status register, returning its value in the location
> + * Return the status register value.
> + * Returns negative if error occurred.
> + */
> +static int read_fsr(struct m25p *flash)
> +{
> +	ssize_t retval;
> +	u8 code = OPCODE_RDFSR;
> +	u8 val;
> +
> +	retval = spi_write_then_read(flash->spi, &code, 1, &val, 1);
> +
> +	if (retval < 0) {
> +		printk(&flash->spi->dev, "error %d reading FSR\n",
> +				(int) retval);
> +		return retval;
> +	}
> +
> +	return val;
> +}
> +
> +/*
>   * Write status register 1 byte
>   * Returns negative if error occurred.
>   */
> @@ -220,6 +246,30 @@ static int wait_till_ready(struct m25p *flash)
>  }
>  
>  /*
> + * Service routine to read flag status register until ready, or timeout occurs.
> + * Returns non-zero if error.
> + */
> +static int wait_till_fsr_ready(struct m25p *flash)
> +{
> +	unsigned long deadline;
> +	int sr;
> +
> +	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> +
> +	do {
> +		if ((sr = read_fsr(flash)) < 0)
> +			break;
> +		else if ((sr & FSR_RDY))
> +			return 0;
> +
> +		cond_resched();
> +
> +	} while (!time_after_eq(jiffies, deadline));
> +
> +	return 1;
> +}
> +
> +/*
>   * Erase the whole flash memory
>   *
>   * Returns 0 if successful, non-zero otherwise.
> @@ -273,6 +323,14 @@ static int erase_sector(struct m25p *flash, u32 offset)
>  	if (wait_till_ready(flash))
>  		return 1;
>  
> +	/* Flag status, Wait until finished previous write command. */
> +	if (flash->flag_status == true) {
> +		if (wait_till_fsr_ready(flash)) {
> +			mutex_unlock(&flash->lock);
> +			return 1;
> +		}
> +	}
> +
>  	/* Send write enable, then erase commands. */
>  	write_enable(flash);
>  
> @@ -427,6 +485,14 @@ static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
>  
>  	mutex_lock(&flash->lock);
>  
> +	/* Flag status, Wait until finished previous write command. */
> +	if (flash->flag_status == true) {
> +		if (wait_till_fsr_ready(flash)) {
> +			mutex_unlock(&flash->lock);
> +			return 1;
> +		}
> +	}
> +
>  	/* Wait until finished previous write command. */
>  	if (wait_till_ready(flash)) {
>  		mutex_unlock(&flash->lock);
> @@ -457,6 +523,14 @@ static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
>  		t[1].len = page_size;
>  		spi_sync(flash->spi, &m);
>  
> +		/* Flag status, Wait until finished previous write command. */
> +		if (flash->flag_status == true) {
> +			if (wait_till_fsr_ready(flash)) {
> +				mutex_unlock(&flash->lock);
> +				return 1;
> +			}
> +		}
> +
>  		*retlen = m.actual_length - m25p_cmdsz(flash);
>  
>  		/* write everything in flash->page_size chunks */
> @@ -477,6 +551,14 @@ static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
>  
>  			spi_sync(flash->spi, &m);
>  
> +			/* Flag status, Wait until finished previous write command. */
> +			if (flash->flag_status == true) {
> +				if (wait_till_fsr_ready(flash)) {
> +					mutex_unlock(&flash->lock);
> +					return 1;
> +				}
> +			}
> +
>  			*retlen += m.actual_length - m25p_cmdsz(flash);
>  		}
>  	}
> @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
>  	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
>  	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
>  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },

You probably want to figure out the distinction between the old table
entry and the new one, and assign your new entry a new string
accordingly.

>  
>  	/* PMC */
>  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> @@ -996,6 +1078,13 @@ static int m25p_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, flash);
>  
>  	/*
> +	 * Micron n25q512a requires to check flag status register
> +	 */
> +	flash->flag_status = false;
> +	if (strcmp(id->name, "n25q512a") == 0)
> +		flash->flag_status = true;;

This doesn't look good at all. If any other flash start to need this, we
don't want to code each ID string here. That's fragile and
non-scaleable. If we need this option, you need to add another flag to
the m25p_ids[] table, I guess...

> +
> +	/*
>  	 * Atmel, SST and Intel/Numonyx serial flash tend to power
>  	 * up with the software protection bits set
>  	 */

Brian
Marek Vasut - Feb. 27, 2014, 8:01 p.m.
On Thursday, February 27, 2014 at 08:33:14 AM, Brian Norris wrote:
> + Marek

Thanks, I really need to subscribe to this ML ;-/

> Hi Insop,
> 
> On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
> > In order to use Micron n25q512a, MTD, two changes are required as
> > follows: - jedec code should be fixed
> 
> I have a feeling there are more than one "n25q512a" device, with
> different IDs.

ACK, I have similar feeling. Jain, can you tell us the precise marking on your 
N25Q512A chip (that is, every single letter on the top of the chip package 
exactly as it is engraved there ) please? Or make a photo ...

Insop, can you please do the same ?

[...]

> > @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
> > 
> >  	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
> >  	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
> >  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> > 
> > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> > +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
> 
> You probably want to figure out the distinction between the old table
> entry and the new one, and assign your new entry a new string
> accordingly.

ACK. The datasheet actually claims this change is correct, see [1] (page 40, 
table 21), but as we know, Micron really does shitty job when it comes to using 
the JEDEC IDs for it's chips.

> >  	/* PMC */
> >  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> > 
> > @@ -996,6 +1078,13 @@ static int m25p_probe(struct spi_device *spi)
> > 
> >  	spi_set_drvdata(spi, flash);
> >  	
> >  	/*
> > 
> > +	 * Micron n25q512a requires to check flag status register
> > +	 */
> > +	flash->flag_status = false;
> > +	if (strcmp(id->name, "n25q512a") == 0)
> > +		flash->flag_status = true;;
> 
> This doesn't look good at all. If any other flash start to need this, we
> don't want to code each ID string here. That's fragile and
> non-scaleable. If we need this option, you need to add another flag to
> the m25p_ids[] table, I guess...

This waiting for some bit in FSR looks like it can be wrapped into 
wait_till_ready(), no ?

What I cannot find in the datasheet though is any evidence which would clearly 
mandate the use of FSR bit 0x80 to test if the chip is ready for next operation 
instead of using the regular STATUS register bit . Insop, can you please 
elaborate on why using the FSR is needed ?

[1] 
https://www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
Insop Song - March 1, 2014, 2 a.m.
Hi Brian,

Thank you for your feedback.

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: Wednesday, February 26, 2014 11:33 PM> 
> 
> On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
> > In order to use Micron n25q512a, MTD, two changes are required as
> follows:
> > - jedec code should be fixed
> 
> I have a feeling there are more than one "n25q512a" device, with different
> IDs.
> 
> > - flag status should be read for writing.
> >
> > Check flag status register for Micron n25q512a
> >     - Programing Micron n25q512a requires to check flag status register
> >     - According to datasheet
> >     	"
> >     	The flag status register must be read any time a PROGRAM, ERASE,
> >     	SUSPEND/RESUME command is issued, or after a RESET command
> while device
> >     	is busy. The cycle is not complete until bit 7 of the flag status
> >     	register output 1.
> >     	"
> >     - Ref:
> >
> https://www.micron.com/~/media/Documents/Products/Data%20Sheet/N
> OR%20F
> > lash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
> 
> Hmm, are you sure that all Micron n25q512a need to check the flag status
> register? I'll check my datasheets when I'm back in the office, but this seems
> peculiar.
> 

"Flag status" term can be found in 84 time in the data sheet and I don't think the flag status is specific to particular type of n25q512a.

> > Signed-off-by: Insop Song <insop.song@gainspeed.com>
> > ---
> >  drivers/mtd/devices/m25p80.c |   91
> +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 90 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c
> > b/drivers/mtd/devices/m25p80.c index 7eda71d..e53e522 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -38,6 +38,7 @@
> >  /* Flash opcodes. */
> >  #define	OPCODE_WREN		0x06	/* Write enable */
> >  #define	OPCODE_RDSR		0x05	/* Read status register */
> > +#define	OPCODE_RDFSR		0x70	/* Read flag status
> register */
> >  #define	OPCODE_WRSR		0x01	/* Write status
> register 1 byte */
> >  #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low
> frequency) */
> >  #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high
> frequency) */
> > @@ -70,6 +71,8 @@
> >  /* Status Register bits. */
> >  #define	SR_WIP			1	/* Write in progress
> */
> >  #define	SR_WEL			2	/* Write enable latch
> */
> > +/* Flag Status Register bits. */
> > +#define	FSR_RDY			0x80	/* FSR ready */
> >  /* meaning of other SR_* bits may differ between vendors */
> >  #define	SR_BP0			4	/* Block protect 0 */
> >  #define	SR_BP1			8	/* Block protect 1 */
> > @@ -95,6 +98,7 @@ struct m25p {
> >  	u8			program_opcode;
> >  	u8			*command;
> >  	bool			fast_read;
> > +	bool			flag_status;
> >  };
> >
> >  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd) @@
> > -131,6 +135,28 @@ static int read_sr(struct m25p *flash)  }
> >
> >  /*
> > + * Read the flag status register, returning its value in the location
> > + * Return the status register value.
> > + * Returns negative if error occurred.
> > + */
> > +static int read_fsr(struct m25p *flash) {
> > +	ssize_t retval;
> > +	u8 code = OPCODE_RDFSR;
> > +	u8 val;
> > +
> > +	retval = spi_write_then_read(flash->spi, &code, 1, &val, 1);
> > +
> > +	if (retval < 0) {
> > +		printk(&flash->spi->dev, "error %d reading FSR\n",
> > +				(int) retval);
> > +		return retval;
> > +	}
> > +
> > +	return val;
> > +}
> > +
> > +/*
> >   * Write status register 1 byte
> >   * Returns negative if error occurred.
> >   */
> > @@ -220,6 +246,30 @@ static int wait_till_ready(struct m25p *flash)  }
> >
> >  /*
> > + * Service routine to read flag status register until ready, or timeout
> occurs.
> > + * Returns non-zero if error.
> > + */
> > +static int wait_till_fsr_ready(struct m25p *flash) {
> > +	unsigned long deadline;
> > +	int sr;
> > +
> > +	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > +
> > +	do {
> > +		if ((sr = read_fsr(flash)) < 0)
> > +			break;
> > +		else if ((sr & FSR_RDY))
> > +			return 0;
> > +
> > +		cond_resched();
> > +
> > +	} while (!time_after_eq(jiffies, deadline));
> > +
> > +	return 1;
> > +}
> > +
> > +/*
> >   * Erase the whole flash memory
> >   *
> >   * Returns 0 if successful, non-zero otherwise.
> > @@ -273,6 +323,14 @@ static int erase_sector(struct m25p *flash, u32
> offset)
> >  	if (wait_till_ready(flash))
> >  		return 1;
> >
> > +	/* Flag status, Wait until finished previous write command. */
> > +	if (flash->flag_status == true) {
> > +		if (wait_till_fsr_ready(flash)) {
> > +			mutex_unlock(&flash->lock);
> > +			return 1;
> > +		}
> > +	}
> > +
> >  	/* Send write enable, then erase commands. */
> >  	write_enable(flash);
> >
> > @@ -427,6 +485,14 @@ static int m25p80_write(struct mtd_info *mtd,
> > loff_t to, size_t len,
> >
> >  	mutex_lock(&flash->lock);
> >
> > +	/* Flag status, Wait until finished previous write command. */
> > +	if (flash->flag_status == true) {
> > +		if (wait_till_fsr_ready(flash)) {
> > +			mutex_unlock(&flash->lock);
> > +			return 1;
> > +		}
> > +	}
> > +
> >  	/* Wait until finished previous write command. */
> >  	if (wait_till_ready(flash)) {
> >  		mutex_unlock(&flash->lock);
> > @@ -457,6 +523,14 @@ static int m25p80_write(struct mtd_info *mtd,
> loff_t to, size_t len,
> >  		t[1].len = page_size;
> >  		spi_sync(flash->spi, &m);
> >
> > +		/* Flag status, Wait until finished previous write command. */
> > +		if (flash->flag_status == true) {
> > +			if (wait_till_fsr_ready(flash)) {
> > +				mutex_unlock(&flash->lock);
> > +				return 1;
> > +			}
> > +		}
> > +
> >  		*retlen = m.actual_length - m25p_cmdsz(flash);
> >
> >  		/* write everything in flash->page_size chunks */ @@ -477,6
> +551,14
> > @@ static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t
> > len,
> >
> >  			spi_sync(flash->spi, &m);
> >
> > +			/* Flag status, Wait until finished previous write
> command. */
> > +			if (flash->flag_status == true) {
> > +				if (wait_till_fsr_ready(flash)) {
> > +					mutex_unlock(&flash->lock);
> > +					return 1;
> > +				}
> > +			}
> > +
> >  			*retlen += m.actual_length - m25p_cmdsz(flash);
> >  		}
> >  	}
> > @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
> >  	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
> >  	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
> >  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> > +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
> 
> You probably want to figure out the distinction between the old table entry
> and the new one, and assign your new entry a new string accordingly.

You mean "0x20bb20" (old value) must be still the valid value?
I will wait to you to add new string.

> 
> >
> >  	/* PMC */
> >  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> > @@ -996,6 +1078,13 @@ static int m25p_probe(struct spi_device *spi)
> >  	spi_set_drvdata(spi, flash);
> >
> >  	/*
> > +	 * Micron n25q512a requires to check flag status register
> > +	 */
> > +	flash->flag_status = false;
> > +	if (strcmp(id->name, "n25q512a") == 0)
> > +		flash->flag_status = true;;
> 
> This doesn't look good at all. If any other flash start to need this, we don't
> want to code each ID string here. That's fragile and non-scaleable. If we need
> this option, you need to add another flag to the m25p_ids[] table, I guess...
> 

I agree, and will see how I can make it more generic.
Also wait to see if you/mail list feedback on this flag status.

> > +
> > +	/*
> >  	 * Atmel, SST and Intel/Numonyx serial flash tend to power
> >  	 * up with the software protection bits set
> >  	 */
> 
> Brian

Insop
Insop Song - March 1, 2014, 2:44 a.m.
Hi Marek,

> From: Marek Vasut [mailto:marex@denx.de]
> Sent: Thursday, February 27, 2014 12:02 PM
> 
> On Thursday, February 27, 2014 at 08:33:14 AM, Brian Norris wrote:
> > + Marek
> 
> Thanks, I really need to subscribe to this ML ;-/
> 
> > Hi Insop,
> >
> > On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
> > > In order to use Micron n25q512a, MTD, two changes are required as
> > > follows: - jedec code should be fixed
> >
> > I have a feeling there are more than one "n25q512a" device, with
> > different IDs.
> 
> ACK, I have similar feeling. Jain, can you tell us the precise marking on your
> N25Q512A chip (that is, every single letter on the top of the chip package
> exactly as it is engraved there ) please? Or make a photo ...
> 
> Insop, can you please do the same ?
> 
> [...]

25Q512A
13640

> 
> > > @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
> > >
> > >  	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
> > >  	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
> > >  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> > >
> > > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> > > +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
> >
> > You probably want to figure out the distinction between the old table
> > entry and the new one, and assign your new entry a new string
> > accordingly.
> 
> ACK. The datasheet actually claims this change is correct, see [1] (page 40,
> table 21), but as we know, Micron really does shitty job when it comes to
> using the JEDEC IDs for it's chips.
> 

"n25q512a1" okay?
Or any other suggestion?

> > >  	/* PMC */
> > >  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> > >
> > > @@ -996,6 +1078,13 @@ static int m25p_probe(struct spi_device *spi)
> > >
> > >  	spi_set_drvdata(spi, flash);
> > >
> > >  	/*
> > >
> > > +	 * Micron n25q512a requires to check flag status register
> > > +	 */
> > > +	flash->flag_status = false;
> > > +	if (strcmp(id->name, "n25q512a") == 0)
> > > +		flash->flag_status = true;;
> >
> > This doesn't look good at all. If any other flash start to need this,
> > we don't want to code each ID string here. That's fragile and
> > non-scaleable. If we need this option, you need to add another flag to
> > the m25p_ids[] table, I guess...
> 
> This waiting for some bit in FSR looks like it can be wrapped into
> wait_till_ready(), no ?
> 
> What I cannot find in the datasheet though is any evidence which would
> clearly mandate the use of FSR bit 0x80 to test if the chip is ready for next
> operation instead of using the regular STATUS register bit . Insop, can you
> please elaborate on why using the FSR is needed ?
> 

If I don't check FSR(Flag Status Register), then I was not able to program this flash.
I've used C SDK from Aardvark I2C/SPI Host Adapter to program directly, and we had to check FSR.
Also in linux driver, I had to put the check otherwise, I am not able to write the flash.
I think it is mandatory though datasheet is not so clear about it.


> [1]
> https://www.micron.com/~/media/Documents/Products/Data%20Sheet/N
> OR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
Chuck Peplinski - March 1, 2014, 2:48 p.m.
We also use this SPI flash and we have a similar mod to make it work.  I 
can't say I like the unique branch in the mtd code, but alternatives are 
more complicated.  Maybe a callback function to check status?

Chuck Peplinski
Vice President of Engineering
Momentum Data Systems


On 2/28/2014 8:44 PM, Insop Song wrote:
> Hi Marek,
>
>> From: Marek Vasut [mailto:marex@denx.de]
>> Sent: Thursday, February 27, 2014 12:02 PM
>>
>> On Thursday, February 27, 2014 at 08:33:14 AM, Brian Norris wrote:
>>> + Marek
>>
>> Thanks, I really need to subscribe to this ML ;-/
>>
>>> Hi Insop,
>>>
>>> On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
>>>> In order to use Micron n25q512a, MTD, two changes are required as
>>>> follows: - jedec code should be fixed
>>>
>>> I have a feeling there are more than one "n25q512a" device, with
>>> different IDs.
>>
>> ACK, I have similar feeling. Jain, can you tell us the precise marking on your
>> N25Q512A chip (that is, every single letter on the top of the chip package
>> exactly as it is engraved there ) please? Or make a photo ...
>>
>> Insop, can you please do the same ?
>>
>> [...]
>
> 25Q512A
> 13640
>
>>
>>>> @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
>>>>
>>>>   	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
>>>>   	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
>>>>   	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
>>>>
>>>> -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
>>>> +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
>>>
>>> You probably want to figure out the distinction between the old table
>>> entry and the new one, and assign your new entry a new string
>>> accordingly.
>>
>> ACK. The datasheet actually claims this change is correct, see [1] (page 40,
>> table 21), but as we know, Micron really does shitty job when it comes to
>> using the JEDEC IDs for it's chips.
>>
>
> "n25q512a1" okay?
> Or any other suggestion?
>
>>>>   	/* PMC */
>>>>   	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
>>>>
>>>> @@ -996,6 +1078,13 @@ static int m25p_probe(struct spi_device *spi)
>>>>
>>>>   	spi_set_drvdata(spi, flash);
>>>>
>>>>   	/*
>>>>
>>>> +	 * Micron n25q512a requires to check flag status register
>>>> +	 */
>>>> +	flash->flag_status = false;
>>>> +	if (strcmp(id->name, "n25q512a") == 0)
>>>> +		flash->flag_status = true;;
>>>
>>> This doesn't look good at all. If any other flash start to need this,
>>> we don't want to code each ID string here. That's fragile and
>>> non-scaleable. If we need this option, you need to add another flag to
>>> the m25p_ids[] table, I guess...
>>
>> This waiting for some bit in FSR looks like it can be wrapped into
>> wait_till_ready(), no ?
>>
>> What I cannot find in the datasheet though is any evidence which would
>> clearly mandate the use of FSR bit 0x80 to test if the chip is ready for next
>> operation instead of using the regular STATUS register bit . Insop, can you
>> please elaborate on why using the FSR is needed ?
>>
>
> If I don't check FSR(Flag Status Register), then I was not able to program this flash.
> I've used C SDK from Aardvark I2C/SPI Host Adapter to program directly, and we had to check FSR.
> Also in linux driver, I had to put the check otherwise, I am not able to write the flash.
> I think it is mandatory though datasheet is not so clear about it.
>
>
>> [1]
>> https://www.micron.com/~/media/Documents/Products/Data%20Sheet/N
>> OR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Marek Vasut - March 1, 2014, 7:01 p.m.
On Saturday, March 01, 2014 at 03:48:17 PM, Chuck Peplinski wrote:
> We also use this SPI flash and we have a similar mod to make it work.  I
> can't say I like the unique branch in the mtd code, but alternatives are
> more complicated.  Maybe a callback function to check status?

Can you please stop top-posting ? Moreover, I really cannot make heads or tails 
of your comment, sorry.

Best regards,
Marek Vasut
Marek Vasut - March 1, 2014, 7:04 p.m.
On Saturday, March 01, 2014 at 03:00:04 AM, Insop Song wrote:
> Hi Brian,
> 
> Thank you for your feedback.
> 
> > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > Sent: Wednesday, February 26, 2014 11:33 PM>
> > 
> > On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
> > > In order to use Micron n25q512a, MTD, two changes are required as
> > 
> > follows:
> > > - jedec code should be fixed
> > 
> > I have a feeling there are more than one "n25q512a" device, with
> > different IDs.
> > 
> > > - flag status should be read for writing.
> > > 
> > > Check flag status register for Micron n25q512a
> > > 
> > >     - Programing Micron n25q512a requires to check flag status register
> > >     - According to datasheet
> > >     
> > >     	"
> > >     	The flag status register must be read any time a PROGRAM, ERASE,
> > >     	SUSPEND/RESUME command is issued, or after a RESET command
> > 
> > while device
> > 
> > >     	is busy. The cycle is not complete until bit 7 of the flag 
status
> > >     	register output 1.
> > >     	"
> > >     
> > >     - Ref:
> > https://www.micron.com/~/media/Documents/Products/Data%20Sheet/N
> > OR%20F
> > 
> > > lash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
> > 
> > Hmm, are you sure that all Micron n25q512a need to check the flag status
> > register? I'll check my datasheets when I'm back in the office, but this
> > seems peculiar.
> 
> "Flag status" term can be found in 84 time in the data sheet and I don't
> think the flag status is specific to particular type of n25q512a.

n25q256a works just fine with regular SR check.

[...]

> > > @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
> > > 
> > >  	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
> > >  	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
> > >  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> > > 
> > > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> > > +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
> > 
> > You probably want to figure out the distinction between the old table
> > entry and the new one, and assign your new entry a new string
> > accordingly.
> 
> You mean "0x20bb20" (old value) must be still the valid value?

Likely, since someone added it before and if you look at the other Micron 
entries, you see chips with '0x20bb..' as well as '0x20ba' . Micron must have 
done something funny here (again :-( )
[...]
Marek Vasut - March 1, 2014, 7:22 p.m.
On Saturday, March 01, 2014 at 03:44:46 AM, Insop Song wrote:
> Hi Marek,
> 
> > From: Marek Vasut [mailto:marex@denx.de]
> > Sent: Thursday, February 27, 2014 12:02 PM
> > 
> > On Thursday, February 27, 2014 at 08:33:14 AM, Brian Norris wrote:
> > > + Marek
> > 
> > Thanks, I really need to subscribe to this ML ;-/
> > 
> > > Hi Insop,
> > > 
> > > On Mon, Jan 06, 2014 at 05:21:17AM +0000, Insop Song wrote:
> > > > In order to use Micron n25q512a, MTD, two changes are required as
> > > > follows: - jedec code should be fixed
> > > 
> > > I have a feeling there are more than one "n25q512a" device, with
> > > different IDs.
> > 
> > ACK, I have similar feeling. Jain, can you tell us the precise marking on
> > your N25Q512A chip (that is, every single letter on the top of the chip
> > package exactly as it is engraved there ) please? Or make a photo ...
> > 
> > Insop, can you please do the same ?
> > 
> > [...]
> 
> 25Q512A
> 13640

OK, I think I figured it out already.

Look at [1] and [2] . The former is 1V8 option, the later is 3V3 option of the 
same N25Q512A SPI NOR. If you look for 'JEDEC-standard 2-byte signature' in the 
datasheets, you will notice there's one that is 'BB20h' for the 1V8 part and 
'BA20h' for the 3V3 part. So I suppose that's the mystery here and this is the 
explanation.

[1] 
www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1_8v_65nm.pdf

[2] 
www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf

> > > > @@ -782,7 +864,7 @@ static const struct spi_device_id m25p_ids[] = {
> > > > 
> > > >  	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
> > > >  	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
> > > >  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> > > > 
> > > > -	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> > > > +	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
> > > 
> > > You probably want to figure out the distinction between the old table
> > > entry and the new one, and assign your new entry a new string
> > > accordingly.
> > 
> > ACK. The datasheet actually claims this change is correct, see [1] (page
> > 40, table 21), but as we know, Micron really does shitty job when it
> > comes to using the JEDEC IDs for it's chips.
> 
> "n25q512a1" okay?
> Or any other suggestion?

According the chapter 'Part Number Ordering Information', the naming scheme here 
should be the same as in case of the n25q128a11/n25q128a13 , since the two 
trailing numbers mean:

1: 1 ... Byte addressability; HOLD pin; Micron XIP
2: 1 ... Vcc = 1.7 to 2.0V
   3 ... Vcc = 2.7 to 3.6V

> > > >  	/* PMC */
> > > >  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) 
},
> > > > 
> > > > @@ -996,6 +1078,13 @@ static int m25p_probe(struct spi_device *spi)
> > > > 
> > > >  	spi_set_drvdata(spi, flash);
> > > >  	
> > > >  	/*
> > > > 
> > > > +	 * Micron n25q512a requires to check flag status register
> > > > +	 */
> > > > +	flash->flag_status = false;
> > > > +	if (strcmp(id->name, "n25q512a") == 0)
> > > > +		flash->flag_status = true;;
> > > 
> > > This doesn't look good at all. If any other flash start to need this,
> > > we don't want to code each ID string here. That's fragile and
> > > non-scaleable. If we need this option, you need to add another flag to
> > > the m25p_ids[] table, I guess...
> > 
> > This waiting for some bit in FSR looks like it can be wrapped into
> > wait_till_ready(), no ?
> > 
> > What I cannot find in the datasheet though is any evidence which would
> > clearly mandate the use of FSR bit 0x80 to test if the chip is ready for
> > next operation instead of using the regular STATUS register bit . Insop,
> > can you please elaborate on why using the FSR is needed ?
> 
> If I don't check FSR(Flag Status Register), then I was not able to program
> this flash. I've used C SDK from Aardvark I2C/SPI Host Adapter to program
> directly, and we had to check FSR. Also in linux driver, I had to put the
> check otherwise, I am not able to write the flash. I think it is mandatory
> though datasheet is not so clear about it.

What kind of problems do you get when you try to write the flash ? I am trying 
to crosscheck this with the N25Q256A I have in here, which also has the FSR, but 
I have no issues programming the chip either in Linux or in U-Boot.

To me, it looks like FSR bit 7 and SR bit 7 should toggle exactly at the same 
time and exactly for the same events. Can you try for example reading them both 
and checking that the bit 7 really toggles at different times please?

Best regards,
Marek Vasut
Chuck Peplinski - March 2, 2014, 5:28 a.m.
Sorry about top posting.
I am also using this n25q512.  My working code includes a fix similar to 
what Song posted.  I don't consider it elegant.  The problem is that 
this 25q512 apparently behaves in a unique fashion.  If you read the 
status register instead of the flag status register, reads work but 
erases and writes fail.  I know this from a couple of days of 
debugging.  You must respond to the flag status register.  Yes, this is 
different from every other part.

Some possible solutions are:
- hard code support for this device, as Song did.
- add some other abstraction that affects support for every other part.
I leave the decision to you, but neither sounds very pretty.

     Chuck

On 3/1/2014 1:22 PM, Marek Vasut wrote:
> To me, it looks like FSR bit 7 and SR bit 7 should toggle exactly at the same
> time and exactly for the same events. Can you try for example reading them both
> and checking that the bit 7 really toggles at different times please?
>
Marek Vasut - March 2, 2014, 2:42 p.m.
On Sunday, March 02, 2014 at 06:28:26 AM, Chuck Peplinski wrote:
> Sorry about top posting.

You are sorry, yet you did it again :-(

> I am also using this n25q512.  My working code includes a fix similar to
> what Song posted.  I don't consider it elegant.  The problem is that
> this 25q512 apparently behaves in a unique fashion.  If you read the
> status register instead of the flag status register, reads work but
> erases and writes fail.  I know this from a couple of days of
> debugging.

So does the SR and FSR not toggle the bit 7 at the same time or what ?

> You must respond to the flag status register.

What do you mean by "respond" ?

> Yes, this is
> different from every other part.
> 
> Some possible solutions are:
> - hard code support for this device, as Song did.
> - add some other abstraction that affects support for every other part.
> I leave the decision to you, but neither sounds very pretty.
> 
>      Chuck
> 
> On 3/1/2014 1:22 PM, Marek Vasut wrote:
> > To me, it looks like FSR bit 7 and SR bit 7 should toggle exactly at the
> > same time and exactly for the same events. Can you try for example
> > reading them both and checking that the bit 7 really toggles at
> > different times please?
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Best regards,
Marek Vasut
Chuck Peplinski - March 3, 2014, 4:52 p.m.
On 3/2/2014 8:42 AM, Marek Vasut wrote:
> On Sunday, March 02, 2014 at 06:28:26 AM, Chuck Peplinski wrote:
>> Sorry about top posting.
> You are sorry, yet you did it again :-(
>
>> I am also using this n25q512.  My working code includes a fix similar to
>> what Song posted.  I don't consider it elegant.  The problem is that
>> this 25q512 apparently behaves in a unique fashion.  If you read the
>> status register instead of the flag status register, reads work but
>> erases and writes fail.  I know this from a couple of days of
>> debugging.
> So does the SR and FSR not toggle the bit 7 at the same time or what ?
>
>> You must respond to the flag status register.
> What do you mean by "respond" ?
>
>> Yes, this is
>> different from every other part.
>>
>> Some possible solutions are:
>> - hard code support for this device, as Song did.
>> - add some other abstraction that affects support for every other part.
>> I leave the decision to you, but neither sounds very pretty.
>>
>>       Chuck
>>
>> On 3/1/2014 1:22 PM, Marek Vasut wrote:
>>> To me, it looks like FSR bit 7 and SR bit 7 should toggle exactly at the
>>> same time and exactly for the same events. Can you try for example
>>> reading them both and checking that the bit 7 really toggles at
>>> different times please?
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> Best regards,
> Marek Vasut
>

I'm learning about top posting.  How about this?

FSR can apparently toggle without SR.  For the device to work, I must 
read and then clear the flag status register.

My "wait_till_done()" (below) is called at the end of m25p80_erase() and 
m25p80_write().
If you would like, I could submit a patch, or just a copy of m25p80.c.

// used with N25Q512A
static int wait_till_done(struct m25p *flash) {
     unsigned long deadline;
     u8 code, fsreg;

     code = OPCODE_RD_FLAG_SR;    // read flag status register
     deadline = jiffies + (1* HZ);  // one second

     do {
         spi_write_then_read(flash->spi, &code, 1, &fsreg, 1);
         if (fsreg & BIT_FLAG_STATUS_READY) {   // not busy.
             code = OPCODE_CLEAR_FLAG_SR;    // clear flag status register
             spi_write_then_read(flash->spi, &code, 1, NULL, 0);
             return 0;
         }
         cond_resched();  // a form of kernel sleep
     } while (!time_after_eq(jiffies, deadline));
     return 1;
}

Chuck
Marek Vasut - March 4, 2014, 12:29 a.m.
On Monday, March 03, 2014 at 05:52:06 PM, Chuck Peplinski wrote:
> On 3/2/2014 8:42 AM, Marek Vasut wrote:
> > On Sunday, March 02, 2014 at 06:28:26 AM, Chuck Peplinski wrote:
> >> Sorry about top posting.
> > 
> > You are sorry, yet you did it again :-(
> > 
> >> I am also using this n25q512.  My working code includes a fix similar to
> >> what Song posted.  I don't consider it elegant.  The problem is that
> >> this 25q512 apparently behaves in a unique fashion.  If you read the
> >> status register instead of the flag status register, reads work but
> >> erases and writes fail.  I know this from a couple of days of
> >> debugging.
> > 
> > So does the SR and FSR not toggle the bit 7 at the same time or what ?
> > 
> >> You must respond to the flag status register.
> > 
> > What do you mean by "respond" ?
> > 
> >> Yes, this is
> >> different from every other part.
> >> 
> >> Some possible solutions are:
> >> - hard code support for this device, as Song did.
> >> - add some other abstraction that affects support for every other part.
> >> I leave the decision to you, but neither sounds very pretty.
> >> 
> >>       Chuck
> >> 
> >> On 3/1/2014 1:22 PM, Marek Vasut wrote:
> >>> To me, it looks like FSR bit 7 and SR bit 7 should toggle exactly at
> >>> the same time and exactly for the same events. Can you try for example
> >>> reading them both and checking that the bit 7 really toggles at
> >>> different times please?
> >> 
> >> ______________________________________________________
> >> Linux MTD discussion mailing list
> >> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> > 
> > Best regards,
> > Marek Vasut
> 
> I'm learning about top posting.  How about this?

Better, good job ;-)

Now , how come the CC list was dropped ?

> FSR can apparently toggle without SR.

Is that documented anywhere ? How can that be ?

> For the device to work, I must
> read and then clear the flag status register.
> 
> My "wait_till_done()" (below) is called at the end of m25p80_erase() and
> m25p80_write().

Hmmmm , I have a feeling that if you actually added wait_till_ready() call at 
the end of _erase() and _write(), you would get the same effect. This would in 
turn mean you are instead missing wait_till_ready() somewhere else.

Can you try using wait_till_ready() at the end of _erase() and _write() please ?

> If you would like, I could submit a patch, or just a copy of m25p80.c.
> 
> // used with N25Q512A
> static int wait_till_done(struct m25p *flash) {
>      unsigned long deadline;
>      u8 code, fsreg;
> 
>      code = OPCODE_RD_FLAG_SR;    // read flag status register
>      deadline = jiffies + (1* HZ);  // one second
> 
>      do {
>          spi_write_then_read(flash->spi, &code, 1, &fsreg, 1);
>          if (fsreg & BIT_FLAG_STATUS_READY) {   // not busy.
>              code = OPCODE_CLEAR_FLAG_SR;    // clear flag status register
>              spi_write_then_read(flash->spi, &code, 1, NULL, 0);
>              return 0;
>          }
>          cond_resched();  // a form of kernel sleep
>      } while (!time_after_eq(jiffies, deadline));
>      return 1;
> }
> 
> Chuck
Chuck Peplinski - March 4, 2014, 9:45 p.m.
On 3/3/2014 6:29 PM, Marek Vasut wrote:
>> FSR can apparently toggle without SR.
> Is that documented anywhere ? How can that be ?
I'm looking at the data sheet for the part, n25q_512mb_1ce3v_65nm.pdf, 
from the Micron web site at 
http://www.micron.com/products/nor-flash/serial-nor-flash#fullPart&236=10.
The comment that led us in this direction is on page 62:

"ERASE Operations
When the operation is in progress, the program or erase controller bit 
of the flag status
register is set to 0. The flag status register must be polled for the 
operation status. When
the operation completes, that bit is cleared to 1.
Note that the flag status register must be polled even if operation 
times out."

> Hmmmm , I have a feeling that if you actually added wait_till_ready() 
> call at the end of _erase() and _write(), you would get the same 
> effect. This would in turn mean you are instead missing 
> wait_till_ready() somewhere else. Can you try using wait_till_ready() 
> at the end of _erase() and _write() please ? 

That would be the standard code, right?  That's where we started and it 
did not work.

At some point I'll try some more tests.  I'm not blocked on this now, so 
it's not totally critical.
One thing I notice:  The web site notes that this part stacks two 256M 
dies.  Maybe that's why it is non-standard?
Sure would be nice to hear from someone at Micron...
Jagannadha Sutradharudu Teki - March 6, 2014, 9:25 a.m.
Hi All,

On Wed, Mar 5, 2014 at 3:15 AM, Chuck Peplinski <chuck@mds.com> wrote:
> On 3/3/2014 6:29 PM, Marek Vasut wrote:
>>>
>>> FSR can apparently toggle without SR.
>>
>> Is that documented anywhere ? How can that be ?
>
> I'm looking at the data sheet for the part, n25q_512mb_1ce3v_65nm.pdf, from
> the Micron web site at
> http://www.micron.com/products/nor-flash/serial-nor-flash#fullPart&236=10.
> The comment that led us in this direction is on page 62:
>
> "ERASE Operations
> When the operation is in progress, the program or erase controller bit of
> the flag status
> register is set to 0. The flag status register must be polled for the
> operation status. When
> the operation completes, that bit is cleared to 1.
> Note that the flag status register must be polled even if operation times
> out."
>
>
>> Hmmmm , I have a feeling that if you actually added wait_till_ready() call
>> at the end of _erase() and _write(), you would get the same effect. This
>> would in turn mean you are instead missing wait_till_ready() somewhere else.
>> Can you try using wait_till_ready() at the end of _erase() and _write()
>> please ?
>
>
> That would be the standard code, right?  That's where we started and it did
> not work.
>
> At some point I'll try some more tests.  I'm not blocked on this now, so
> it's not totally critical.
> One thing I notice:  The web site notes that this part stacks two 256M dies.
> Maybe that's why it is non-standard?
> Sure would be nice to hear from someone at Micron...

BTW: I have some information regarding this part.

We have tested this part both in u-boot and Linux where we could see
an issue while polling read_status, and then we moved to use flag_status
then we saw the consistent behavior.

Inputs we're got from Micron are:
1. Micron (though not clearly documented), stated that flag_status is clearly
    required to be polled, even though read_status reports the exact status.
    They are not willing to change the datasheet but plan on releasing an
    app note or something because this has led to a lot of confusion.
2. At this moment, no one else has such weird design of needing an
flag_status poll.
    So checking for the device ID and polling flag_status looks like
the only option.

The same approach we tried as well.

CCing - Harini (tested flag_status on Linux)
CCing - Todd Legler from Micron.

Todd and Harini - Please share your inputs.


thanks!
Harini Katakam - March 6, 2014, 10:03 a.m.
Hi,

> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki@gmail.com]
> Sent: Thursday, March 06, 2014 2:56 PM
> To: chuck@mds.com
> Cc: Marek Vasut; linux-mtd@lists.infradead.org; Todd Legler (tlegler); Harini
> Katakam
> Subject: Re: [PATCH] Check flag status register for Micron n25q512a
>
> Hi All,
>
> On Wed, Mar 5, 2014 at 3:15 AM, Chuck Peplinski <chuck@mds.com> wrote:
> > On 3/3/2014 6:29 PM, Marek Vasut wrote:
> >>>
> >>> FSR can apparently toggle without SR.
> >>
> >> Is that documented anywhere ? How can that be ?
> >
> > I'm looking at the data sheet for the part, n25q_512mb_1ce3v_65nm.pdf,
> > from the Micron web site at
> > http://www.micron.com/products/nor-flash/serial-nor-
> flash#fullPart&236=10.
> > The comment that led us in this direction is on page 62:
> >
> > "ERASE Operations
> > When the operation is in progress, the program or erase controller bit
> > of the flag status register is set to 0. The flag status register must
> > be polled for the operation status. When the operation completes, that
> > bit is cleared to 1.
> > Note that the flag status register must be polled even if operation
> > times out."
> >
> >
> >> Hmmmm , I have a feeling that if you actually added wait_till_ready()
> >> call at the end of _erase() and _write(), you would get the same
> >> effect. This would in turn mean you are instead missing wait_till_ready()
> somewhere else.
> >> Can you try using wait_till_ready() at the end of _erase() and
> >> _write() please ?
> >
> >
> > That would be the standard code, right?  That's where we started and
> > it did not work.
> >
> > At some point I'll try some more tests.  I'm not blocked on this now,
> > so it's not totally critical.
> > One thing I notice:  The web site notes that this part stacks two 256M dies.
> > Maybe that's why it is non-standard?
> > Sure would be nice to hear from someone at Micron...
>
> BTW: I have some information regarding this part.
>
> We have tested this part both in u-boot and Linux where we could see an
> issue while polling read_status, and then we moved to use flag_status then
> we saw the consistent behavior.
>
> Inputs we're got from Micron are:
> 1. Micron (though not clearly documented), stated that flag_status is clearly
>     required to be polled, even though read_status reports the exact status.
>     They are not willing to change the datasheet but plan on releasing an
>     app note or something because this has led to a lot of confusion.
> 2. At this moment, no one else has such weird design of needing an
> flag_status poll.
>     So checking for the device ID and polling flag_status looks like the only
> option.
>
> The same approach we tried as well.
>
> CCing - Harini (tested flag_status on Linux) CCing - Todd Legler from Micron.
>
> Todd and Harini - Please share your inputs.
>

Micron devices with more than one die i.e. n25q_512mb and n25q_1gb
have this additional check.
Notes 14 and 15 under command definition table 18 describe this best.
Although, it is stated that the WIP bit in status register is NOT of
bit 7 in Flag status register, FSR still needs to be polled.

We had a discussion with Micron from which we learnt the following:
1. Program/erase operations in multi-die devices are not complete until
Read FSR command receives a ready response form flash at least one time.
2. In addition, if you are doing a Write Non-volatile configuration register/
Write status register operation, the operation is only complete after
FSR poll returns ready consecutively 2 or 4 times (as many times as the number of die).
In this case, each time the chip select is toggled and Read FSR command is sent,
response is sent by alternate die (1,2,1,2... or 1,2,3,4,1,2,3,4.....).

Todd, please add to this if you would like to elaborate.

Regards,
Harini

>
> thanks!
> --
> Jagan.



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Marek Vasut - March 6, 2014, 11:53 a.m.
On Thursday, March 06, 2014 at 11:03:50 AM, Harini Katakam wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Jagan Teki [mailto:jagannadh.teki@gmail.com]
> > Sent: Thursday, March 06, 2014 2:56 PM
> > To: chuck@mds.com
> > Cc: Marek Vasut; linux-mtd@lists.infradead.org; Todd Legler (tlegler);
> > Harini Katakam
> > Subject: Re: [PATCH] Check flag status register for Micron n25q512a
> > 
> > Hi All,
> > 
> > On Wed, Mar 5, 2014 at 3:15 AM, Chuck Peplinski <chuck@mds.com> wrote:
> > > On 3/3/2014 6:29 PM, Marek Vasut wrote:
> > >>> FSR can apparently toggle without SR.
> > >> 
> > >> Is that documented anywhere ? How can that be ?
> > > 
> > > I'm looking at the data sheet for the part, n25q_512mb_1ce3v_65nm.pdf,
> > > from the Micron web site at
> > > http://www.micron.com/products/nor-flash/serial-nor-
> > 
> > flash#fullPart&236=10.
> > 
> > > The comment that led us in this direction is on page 62:
> > > 
> > > "ERASE Operations
> > > When the operation is in progress, the program or erase controller bit
> > > of the flag status register is set to 0. The flag status register must
> > > be polled for the operation status. When the operation completes, that
> > > bit is cleared to 1.
> > > Note that the flag status register must be polled even if operation
> > > times out."
> > > 
> > >> Hmmmm , I have a feeling that if you actually added wait_till_ready()
> > >> call at the end of _erase() and _write(), you would get the same
> > >> effect. This would in turn mean you are instead missing
> > >> wait_till_ready()
> > 
> > somewhere else.
> > 
> > >> Can you try using wait_till_ready() at the end of _erase() and
> > >> _write() please ?
> > > 
> > > That would be the standard code, right?  That's where we started and
> > > it did not work.
> > > 
> > > At some point I'll try some more tests.  I'm not blocked on this now,
> > > so it's not totally critical.
> > > One thing I notice:  The web site notes that this part stacks two 256M
> > > dies. Maybe that's why it is non-standard?
> > > Sure would be nice to hear from someone at Micron...
> > 
> > BTW: I have some information regarding this part.
> > 
> > We have tested this part both in u-boot and Linux where we could see an
> > issue while polling read_status, and then we moved to use flag_status
> > then we saw the consistent behavior.
> > 
> > Inputs we're got from Micron are:
> > 1. Micron (though not clearly documented), stated that flag_status is
> > clearly
> > 
> >     required to be polled, even though read_status reports the exact
> >     status. They are not willing to change the datasheet but plan on
> >     releasing an app note or something because this has led to a lot of
> >     confusion.
> > 
> > 2. At this moment, no one else has such weird design of needing an
> > flag_status poll.
> > 
> >     So checking for the device ID and polling flag_status looks like the
> >     only
> > 
> > option.
> > 
> > The same approach we tried as well.
> > 
> > CCing - Harini (tested flag_status on Linux) CCing - Todd Legler from
> > Micron.
> > 
> > Todd and Harini - Please share your inputs.
> 
> Micron devices with more than one die i.e. n25q_512mb and n25q_1gb
> have this additional check.
> Notes 14 and 15 under command definition table 18 describe this best.
> Although, it is stated that the WIP bit in status register is NOT of
> bit 7 in Flag status register, FSR still needs to be polled.
> 
> We had a discussion with Micron from which we learnt the following:
> 1. Program/erase operations in multi-die devices are not complete until
> Read FSR command receives a ready response form flash at least one time.
> 2. In addition, if you are doing a Write Non-volatile configuration
> register/ Write status register operation, the operation is only complete
> after FSR poll returns ready consecutively 2 or 4 times (as many times as
> the number of die). In this case, each time the chip select is toggled and
> Read FSR command is sent, response is sent by alternate die (1,2,1,2... or
> 1,2,3,4,1,2,3,4.....).
> 
> Todd, please add to this if you would like to elaborate.

Thanks for pointing this out.
Yves Deweerdt - April 18, 2014, 3:07 p.m.
Insop Song,

In the datasheet of the N25Q512A it is mentioning at different places that 
the Flag Status Register should be polled twice in different spi accesses, 
once for each die in the chip. However, I don't see this happening in your 
patch.

- Didn't you have any problems using your patch?
- Is there some git repo having a patch to support the N25Q512A yet?

Kind regards,

Yves

Patch

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7eda71d..e53e522 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -38,6 +38,7 @@ 
 /* Flash opcodes. */
 #define	OPCODE_WREN		0x06	/* Write enable */
 #define	OPCODE_RDSR		0x05	/* Read status register */
+#define	OPCODE_RDFSR		0x70	/* Read flag status register */
 #define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
 #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
 #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
@@ -70,6 +71,8 @@ 
 /* Status Register bits. */
 #define	SR_WIP			1	/* Write in progress */
 #define	SR_WEL			2	/* Write enable latch */
+/* Flag Status Register bits. */
+#define	FSR_RDY			0x80	/* FSR ready */
 /* meaning of other SR_* bits may differ between vendors */
 #define	SR_BP0			4	/* Block protect 0 */
 #define	SR_BP1			8	/* Block protect 1 */
@@ -95,6 +98,7 @@  struct m25p {
 	u8			program_opcode;
 	u8			*command;
 	bool			fast_read;
+	bool			flag_status;
 };
 
 static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -131,6 +135,28 @@  static int read_sr(struct m25p *flash)
 }
 
 /*
+ * Read the flag status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */
+static int read_fsr(struct m25p *flash)
+{
+	ssize_t retval;
+	u8 code = OPCODE_RDFSR;
+	u8 val;
+
+	retval = spi_write_then_read(flash->spi, &code, 1, &val, 1);
+
+	if (retval < 0) {
+		printk(&flash->spi->dev, "error %d reading FSR\n",
+				(int) retval);
+		return retval;
+	}
+
+	return val;
+}
+
+/*
  * Write status register 1 byte
  * Returns negative if error occurred.
  */
@@ -220,6 +246,30 @@  static int wait_till_ready(struct m25p *flash)
 }
 
 /*
+ * Service routine to read flag status register until ready, or timeout occurs.
+ * Returns non-zero if error.
+ */
+static int wait_till_fsr_ready(struct m25p *flash)
+{
+	unsigned long deadline;
+	int sr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		if ((sr = read_fsr(flash)) < 0)
+			break;
+		else if ((sr & FSR_RDY))
+			return 0;
+
+		cond_resched();
+
+	} while (!time_after_eq(jiffies, deadline));
+
+	return 1;
+}
+
+/*
  * Erase the whole flash memory
  *
  * Returns 0 if successful, non-zero otherwise.
@@ -273,6 +323,14 @@  static int erase_sector(struct m25p *flash, u32 offset)
 	if (wait_till_ready(flash))
 		return 1;
 
+	/* Flag status, Wait until finished previous write command. */
+	if (flash->flag_status == true) {
+		if (wait_till_fsr_ready(flash)) {
+			mutex_unlock(&flash->lock);
+			return 1;
+		}
+	}
+
 	/* Send write enable, then erase commands. */
 	write_enable(flash);
 
@@ -427,6 +485,14 @@  static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 	mutex_lock(&flash->lock);
 
+	/* Flag status, Wait until finished previous write command. */
+	if (flash->flag_status == true) {
+		if (wait_till_fsr_ready(flash)) {
+			mutex_unlock(&flash->lock);
+			return 1;
+		}
+	}
+
 	/* Wait until finished previous write command. */
 	if (wait_till_ready(flash)) {
 		mutex_unlock(&flash->lock);
@@ -457,6 +523,14 @@  static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
 		t[1].len = page_size;
 		spi_sync(flash->spi, &m);
 
+		/* Flag status, Wait until finished previous write command. */
+		if (flash->flag_status == true) {
+			if (wait_till_fsr_ready(flash)) {
+				mutex_unlock(&flash->lock);
+				return 1;
+			}
+		}
+
 		*retlen = m.actual_length - m25p_cmdsz(flash);
 
 		/* write everything in flash->page_size chunks */
@@ -477,6 +551,14 @@  static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 			spi_sync(flash->spi, &m);
 
+			/* Flag status, Wait until finished previous write command. */
+			if (flash->flag_status == true) {
+				if (wait_till_fsr_ready(flash)) {
+					mutex_unlock(&flash->lock);
+					return 1;
+				}
+			}
+
 			*retlen += m.actual_length - m25p_cmdsz(flash);
 		}
 	}
@@ -782,7 +864,7 @@  static const struct spi_device_id m25p_ids[] = {
 	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
-	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
+	{ "n25q512a",    INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K) },
 
 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
@@ -996,6 +1078,13 @@  static int m25p_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, flash);
 
 	/*
+	 * Micron n25q512a requires to check flag status register
+	 */
+	flash->flag_status = false;
+	if (strcmp(id->name, "n25q512a") == 0)
+		flash->flag_status = true;;
+
+	/*
 	 * Atmel, SST and Intel/Numonyx serial flash tend to power
 	 * up with the software protection bits set
 	 */