| Message ID | 1397228593-17996-1-git-send-email-grmoore@altera.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Apr 11, 2014 at 10:03:13AM -0500, grmoore@altera.com wrote: > From: Graham Moore <grmoore@altera.com> > > Some new Micron flash chips require reading the flag > status register to determine when operations have completed. > > Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also > require reading the status register before reading the flag status register. > > This patch adds support for the flag status register in the n25q512a1 and n25q00 > Micron QSPI flash chips. > > Signed-off-by: Graham Moore <grmoore@altera.com> > --- > V2: > Remove leading underscore in function names. > Remove type cast in dev_err call and use the proper format > specifier instead. > --- > drivers/mtd/devices/m25p80.c | 91 ++++++++++++++++++++++++++++++++++++------ please rebase this patch on the latest l2-mtd, branch spinor. thanks Huang Shijie
On Friday, April 11, 2014 at 05:03:13 PM, grmoore@altera.com wrote: [...] > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index ad19139..4cddbc8 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -39,6 +39,7 @@ > #define OPCODE_WREN 0x06 /* Write enable */ > #define OPCODE_RDSR 0x05 /* Read status register */ > #define OPCODE_WRSR 0x01 /* Write status register 1 byte */ > +#define OPCODE_RDFSR 0x70 /* read flag status register */ I know this is not your fault, but can you please indent this properly with tabs? > #define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ > #define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ > #define OPCODE_QUAD_READ 0x6b /* Read data bytes */ And fix this one in a separate patch to use tabs as well please ? [...] > @@ -790,7 +847,7 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t > ofs, uint64_t len) > > mutex_lock(&flash->lock); > /* Wait until finished previous command */ > - if (wait_till_ready(flash)) { > + if (flash->wait_till_ready(flash)) { I wonder, can't wait_till_ready() be made a wrapper that will check the USE_FSR flag and call correct wait-function ? This would avoid adding a new member to *flash and also would avoid so many changes throughout the code. What do you think? [...] Best regards, Marek Vasut
Hi, On Fri, Apr 11, 2014 at 8:33 PM, <grmoore@altera.com> wrote: > From: Graham Moore <grmoore@altera.com> > > Some new Micron flash chips require reading the flag > status register to determine when operations have completed. > > Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also > require reading the status register before reading the flag status register. > > This patch adds support for the flag status register in the n25q512a1 and n25q00 > Micron QSPI flash chips. > > Signed-off-by: Graham Moore <grmoore@altera.com> <snip> > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ > @@ -941,6 +999,8 @@ static const struct spi_device_id m25p_ids[] = { > { "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) }, > + { "n25q512a1", INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) }, > + { "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) }, > I understand that "n25q512a1" was added to distinguish between 0x20bb20 and 0x20ba20, which is essentially 1.8V and 3V parts. (The actual part numbers are n25q512a11 and n25q512a13 respectively) But USE_FSR is required for both parts. Sorry for posting this question here but it seemed relevant: When such devices differ only in supply voltages (and return different response to READ ID), which we don't act on, is there a way to use the same string? Regards, Harini
On Sun, Apr 13, 2014 at 12:18 PM, Marek Vasut <marex@denx.de> wrote: [...] >> +#define OPCODE_RDFSR 0x70 /* read flag status register */ > > I know this is not your fault, but can you please indent this properly with > tabs? > >> #define OPCODE_NORM_READ 0x03 /* Read data bytes (low > frequency) */ >> #define OPCODE_FAST_READ 0x0b /* Read data bytes (high > frequency) */ >> #define OPCODE_QUAD_READ 0x6b /* Read data bytes */ > > And fix this one in a separate patch to use tabs as well please ? I'm rebasing on l2-mtd spinor, and the tabs are jacked up there too. I'll fix the one I added, and fix other tabs in a new patch on that branch. [...] > I wonder, can't wait_till_ready() be made a wrapper that will check the USE_FSR > flag and call correct wait-function ? This would avoid adding a new member to > *flash and also would avoid so many changes throughout the code. What do you > think? Yeah, that's kind of ugly, but the flags exist only in the static m25p_ids[] and are only used in the scan/init. I'd have to add a new member to save them for later use. And then other uses would crop up. So, seemed kinda iffy either way. It's going to change anyway, because l2-mtd spinor branch has refactored wait_till_ready() such that it uses a function pointer from the new spi_nor struct. I'm still mulling it over :) Thanks, -Graham
On Monday, April 14, 2014 at 05:41:34 PM, Harini Katakam wrote: > Hi, > > On Fri, Apr 11, 2014 at 8:33 PM, <grmoore@altera.com> wrote: > > From: Graham Moore <grmoore@altera.com> > > > > Some new Micron flash chips require reading the flag > > status register to determine when operations have completed. > > > > Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also > > require reading the status register before reading the flag status > > register. > > > > This patch adds support for the flag status register in the n25q512a1 and > > n25q00 Micron QSPI flash chips. > > > > Signed-off-by: Graham Moore <grmoore@altera.com> > > <snip> > > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ > > > > @@ -941,6 +999,8 @@ static const struct spi_device_id m25p_ids[] = { > > > > { "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) }, > > > > + { "n25q512a1", INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) }, > > + { "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) }, > > I understand that "n25q512a1" was added to distinguish between > 0x20bb20 and 0x20ba20, > which is essentially 1.8V and 3V parts. > (The actual part numbers are n25q512a11 and n25q512a13 respectively) > But USE_FSR is required for both parts. Thanks for noticing it, n25q512aX must be aligned with the other parts' naming scheme as that's the naming scheme used in micron datasheets. > Sorry for posting this question here but it seemed relevant: > When such devices differ only in supply voltages (and return different > response to READ ID), > which we don't act on, is there a way to use the same string? No, they are different chips, so we must not use the same string. Best regards, Marek Vasut
On Monday, April 14, 2014 at 06:19:24 PM, Graham Moore wrote: > On Sun, Apr 13, 2014 at 12:18 PM, Marek Vasut <marex@denx.de> wrote: > [...] > > >> +#define OPCODE_RDFSR 0x70 /* read flag status register > >> */ > > > > I know this is not your fault, but can you please indent this properly > > with tabs? > > > >> #define OPCODE_NORM_READ 0x03 /* Read data bytes (low > > > > frequency) */ > > > >> #define OPCODE_FAST_READ 0x0b /* Read data bytes (high > > > > frequency) */ > > > >> #define OPCODE_QUAD_READ 0x6b /* Read data bytes */ > > > > And fix this one in a separate patch to use tabs as well please ? > > I'm rebasing on l2-mtd spinor, and the tabs are jacked up there too. > I'll fix the > one I added, and fix other tabs in a new patch on that branch. I know :( , thanks! > [...] > > > I wonder, can't wait_till_ready() be made a wrapper that will check the > > USE_FSR flag and call correct wait-function ? This would avoid adding a > > new member to *flash and also would avoid so many changes throughout the > > code. What do you think? > > Yeah, that's kind of ugly, but the flags exist only in the static > m25p_ids[] and are only > used in the scan/init. I'd have to add a new member to save them for > later use. > And then other uses would crop up. So, seemed kinda iffy either way. > > It's going to change anyway, because l2-mtd spinor branch has refactored > wait_till_ready() such that it uses a function pointer from the new > spi_nor struct. > I'm still mulling it over :) Roger that, I'll wait for the V3 . Thanks! Best regards, Marek Vasut
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index ad19139..4cddbc8 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -39,6 +39,7 @@ #define OPCODE_WREN 0x06 /* Write enable */ #define OPCODE_RDSR 0x05 /* Read status register */ #define OPCODE_WRSR 0x01 /* Write status register 1 byte */ +#define OPCODE_RDFSR 0x70 /* read flag status register */ #define OPCODE_NORM_READ 0x03 /* Read data bytes (low frequency) */ #define OPCODE_FAST_READ 0x0b /* Read data bytes (high frequency) */ #define OPCODE_QUAD_READ 0x6b /* Read data bytes */ @@ -81,6 +82,9 @@ #define SR_QUAD_EN_MX 0x40 /* Macronix Quad I/O */ +/* Flag Status Register bits */ +#define FSR_READY 0x80 /* FSR ready */ + /* Configuration Register bits. */ #define CR_QUAD_EN_SPAN 0x2 /* Spansion Quad I/O */ @@ -109,6 +113,7 @@ struct m25p { u8 program_opcode; u8 *command; enum read_type flash_read; + int (*wait_till_ready)(struct m25p *flash); }; static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd) @@ -145,6 +150,27 @@ 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) { + dev_err(&flash->spi->dev, "error %zd reading FSR\n", + retval); + return retval; + } + + return val; +} +/* * Read configuration register, returning its value in the * location. Return the configuration register value. * Returns negative if error occured. @@ -254,6 +280,37 @@ 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 fsr; + int sr; + + deadline = jiffies + MAX_READY_WAIT_JIFFIES; + + do { + sr = read_sr(flash); + if (sr < 0) + break; + /* only check fsr if sr not busy */ + if (!(sr & SR_WIP)) { + fsr = read_fsr(flash); + if (fsr < 0) + break; + if (fsr & FSR_READY) + return 0; + } + + cond_resched(); + + } while (!time_after_eq(jiffies, deadline)); + + return 1; +} +/* * Write status Register and configuration register with 2 bytes * The first byte will be written to the status register, while the * second byte will be written to the configuration register. @@ -280,7 +337,7 @@ static int macronix_quad_enable(struct m25p *flash) spi_write(flash->spi, &cmd, 2); - if (wait_till_ready(flash)) + if (flash->wait_till_ready(flash)) return 1; ret = read_sr(flash); @@ -351,7 +408,7 @@ static int erase_chip(struct m25p *flash) (long long)(flash->mtd.size >> 10)); /* Wait until finished previous write command. */ - if (wait_till_ready(flash)) + if (flash->wait_till_ready(flash)) return 1; /* Send write enable, then erase commands. */ @@ -391,7 +448,7 @@ static int erase_sector(struct m25p *flash, u32 offset) __func__, flash->mtd.erasesize / 1024, offset); /* Wait until finished previous write command. */ - if (wait_till_ready(flash)) + if (flash->wait_till_ready(flash)) return 1; /* Send write enable, then erase commands. */ @@ -536,7 +593,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len, mutex_lock(&flash->lock); /* Wait till previous write/erase is done. */ - if (wait_till_ready(flash)) { + if (flash->wait_till_ready(flash)) { /* REVISIT status return?? */ mutex_unlock(&flash->lock); return 1; @@ -585,7 +642,7 @@ static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len, mutex_lock(&flash->lock); /* Wait until finished previous write command. */ - if (wait_till_ready(flash)) { + if (flash->wait_till_ready(flash)) { mutex_unlock(&flash->lock); return 1; } @@ -628,7 +685,7 @@ static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len, t[1].tx_buf = buf + i; t[1].len = page_size; - wait_till_ready(flash); + flash->wait_till_ready(flash); write_enable(flash); @@ -668,7 +725,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, mutex_lock(&flash->lock); /* Wait until finished previous write command. */ - ret = wait_till_ready(flash); + ret = flash->wait_till_ready(flash); if (ret) goto time_out; @@ -683,7 +740,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, /* write one byte. */ t[1].len = 1; spi_sync(flash->spi, &m); - ret = wait_till_ready(flash); + ret = flash->wait_till_ready(flash); if (ret) goto time_out; *retlen += m.actual_length - m25p_cmdsz(flash); @@ -702,7 +759,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, t[1].tx_buf = buf + actual; spi_sync(flash->spi, &m); - ret = wait_till_ready(flash); + ret = flash->wait_till_ready(flash); if (ret) goto time_out; *retlen += m.actual_length - cmd_sz; @@ -710,7 +767,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, to += 2; } write_disable(flash); - ret = wait_till_ready(flash); + ret = flash->wait_till_ready(flash); if (ret) goto time_out; @@ -724,7 +781,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, t[1].tx_buf = buf + actual; spi_sync(flash->spi, &m); - ret = wait_till_ready(flash); + ret = flash->wait_till_ready(flash); if (ret) goto time_out; *retlen += m.actual_length - m25p_cmdsz(flash); @@ -745,7 +802,7 @@ static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) mutex_lock(&flash->lock); /* Wait until finished previous command */ - if (wait_till_ready(flash)) { + if (flash->wait_till_ready(flash)) { res = 1; goto err; } @@ -790,7 +847,7 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) mutex_lock(&flash->lock); /* Wait until finished previous command */ - if (wait_till_ready(flash)) { + if (flash->wait_till_ready(flash)) { res = 1; goto err; } @@ -856,6 +913,7 @@ struct flash_info { #define M25P_NO_FR 0x08 /* Can't do fastread */ #define SECT_4K_PMC 0x10 /* OPCODE_BE_4K_PMC works uniformly */ #define M25P80_QUAD_READ 0x20 /* Flash supports Quad Read */ +#define USE_FSR 0x40 /* use flag status register */ }; #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ @@ -941,6 +999,8 @@ static const struct spi_device_id m25p_ids[] = { { "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) }, + { "n25q512a1", INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) }, + { "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) }, /* PMC */ { "pm25lv512", INFO(0, 0, 32 * 1024, 2, SECT_4K_PMC) }, @@ -1206,6 +1266,11 @@ static int m25p_probe(struct spi_device *spi) if (info->flags & M25P_NO_ERASE) flash->mtd.flags |= MTD_NO_ERASE; + if (info->flags & USE_FSR) + flash->wait_till_ready = &wait_till_fsr_ready; + else + flash->wait_till_ready = &wait_till_ready; + ppdata.of_node = spi->dev.of_node; flash->mtd.dev.parent = &spi->dev; flash->page_size = info->page_size;