* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-25 2:42 ` Marek Vasut
0 siblings, 0 replies; 40+ messages in thread
From: Marek Vasut @ 2014-04-25 2:42 UTC (permalink / raw)
To: Huang Shijie
Cc: ggrahammoore, Graham Moore, Sascha Hauer, Jingoo Han,
Geert Uytterhoeven, linux-kernel, Yves Vandervennet, linux-mtd,
Artem Bityutskiy, Alan Tull, Sourav Poddar, Brian Norris,
Insop Song, David Woodhouse, Dinh Nguyen
On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > 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 n25q512ax3
> > and n25q00 Micron QSPI flash chips.
> >
> > Signed-off-by: Graham Moore <grmoore@altera.com>
> > ---
> > V3:
> > Rebase to l2-mtd spinor branch.
> > V2:
> > Remove leading underscore in function names.
> > Remove type cast in dev_err call and use the proper format
> > specifier instead.
> > ---
> >
> > drivers/mtd/spi-nor/spi-nor.c | 51
> > +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h
> > | 4 ++++
> > 2 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> >
> > }
> >
> > /*
> >
> > + * 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 spi_nor *nor)
> > +{
> > + int ret;
> > + u8 val;
> > +
> > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > + if (ret < 0) {
> > + pr_err("error %d reading FSR\n", ret);
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
> > +
> > +/*
> >
> > * Read configuration register, returning its value in the
> > * location. Return the configuration register value.
> > * Returns negative if error occured.
> >
> > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
> > *nor)
> >
> > return -ETIMEDOUT;
> >
> > }
> >
> > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > +{
> > + unsigned long deadline;
> > + int sr;
> > + int fsr;
> > +
> > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > +
> > + do {
> > + cond_resched();
> > +
> > + sr = read_sr(nor);
> > + if (sr < 0)
> > + break;
> > + else if (!(sr & SR_WIP)) {
> > + fsr = read_fsr(nor);
> > + if (fsr < 0)
> > + break;
> > + if (fsr & FSR_READY)
> > + return 0;
> > + }
> > + } while (!time_after_eq(jiffies, deadline));
> > +
> > + return -ETIMEDOUT;
> > +}
> > +
> >
> > /*
> >
> > * Service routine to read status register until ready, or timeout
> > occurs. * Returns non-zero if error.
> >
> > @@ -402,6 +447,7 @@ struct flash_info {
> >
> > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC works
uniformly */
> > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */
> > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */
> >
> > +#define USE_FSR 0x80 /* use flag status register */
> >
> > };
> >
> > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> >
> > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> >
> > + { "n25q512ax3", 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) },
> >
> > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
> > spi_device_id *id,
> >
> > else
> >
> > mtd->_write = spi_nor_write;
> >
> > + if (info->flags & USE_FSR)
> > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > +
>
> the drivers may fills this hook itself, so the code should like this:
> --------------------------------------------------
> if ((info->flags & USE_FSR) &&
> nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> --------------------------------------------------
I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver
compete for a function pointer here ? I guess one should have precedence in some
way then ... and also, they should be two different pointers, where the
subsystem decides which to use.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-25 2:42 ` Marek Vasut
@ 2014-04-25 1:52 ` Huang Shijie
-1 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-25 1:52 UTC (permalink / raw)
To: Marek Vasut
Cc: Graham Moore, ggrahammoore, Geert Uytterhoeven, Artem Bityutskiy,
Sascha Hauer, Jingoo Han, linux-kernel, Yves Vandervennet,
linux-mtd, Insop Song, Alan Tull, Sourav Poddar, Brian Norris,
David Woodhouse, Dinh Nguyen
On Fri, Apr 25, 2014 at 04:42:33AM +0200, Marek Vasut wrote:
> On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> > On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > > 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 n25q512ax3
> > > and n25q00 Micron QSPI flash chips.
> > >
> > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > ---
> > > V3:
> > > Rebase to l2-mtd spinor branch.
> > > V2:
> > > Remove leading underscore in function names.
> > > Remove type cast in dev_err call and use the proper format
> > > specifier instead.
> > > ---
> > >
> > > drivers/mtd/spi-nor/spi-nor.c | 51
> > > +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h
> > > | 4 ++++
> > > 2 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> > >
> > > }
> > >
> > > /*
> > >
> > > + * 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 spi_nor *nor)
> > > +{
> > > + int ret;
> > > + u8 val;
> > > +
> > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > > + if (ret < 0) {
> > > + pr_err("error %d reading FSR\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return val;
> > > +}
> > > +
> > > +/*
> > >
> > > * Read configuration register, returning its value in the
> > > * location. Return the configuration register value.
> > > * Returns negative if error occured.
> > >
> > > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
> > > *nor)
> > >
> > > return -ETIMEDOUT;
> > >
> > > }
> > >
> > > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > > +{
> > > + unsigned long deadline;
> > > + int sr;
> > > + int fsr;
> > > +
> > > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > > +
> > > + do {
> > > + cond_resched();
> > > +
> > > + sr = read_sr(nor);
> > > + if (sr < 0)
> > > + break;
> > > + else if (!(sr & SR_WIP)) {
> > > + fsr = read_fsr(nor);
> > > + if (fsr < 0)
> > > + break;
> > > + if (fsr & FSR_READY)
> > > + return 0;
> > > + }
> > > + } while (!time_after_eq(jiffies, deadline));
> > > +
> > > + return -ETIMEDOUT;
> > > +}
> > > +
> > >
> > > /*
> > >
> > > * Service routine to read status register until ready, or timeout
> > > occurs. * Returns non-zero if error.
> > >
> > > @@ -402,6 +447,7 @@ struct flash_info {
> > >
> > > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC works
> uniformly */
> > > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */
> > > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */
> > >
> > > +#define USE_FSR 0x80 /* use flag status register */
> > >
> > > };
> > >
> > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > >
> > > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> > >
> > > + { "n25q512ax3", 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) },
> > >
> > > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
> > > spi_device_id *id,
> > >
> > > else
> > >
> > > mtd->_write = spi_nor_write;
> > >
> > > + if (info->flags & USE_FSR)
> > > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > +
> >
> > the drivers may fills this hook itself, so the code should like this:
> > --------------------------------------------------
> > if ((info->flags & USE_FSR) &&
> > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > --------------------------------------------------
>
> I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver
> compete for a function pointer here ? I guess one should have precedence in some
> way then ... and also, they should be two different pointers, where the
> subsystem decides which to use.
the subsystem do not decides which one to use, the driver decides which one to use.
If driver has its own @wait_till_ready , it means the driver knows the feature,
and has implemented it in its own @wait_till_ready.
If the driver does not fill any wait_till_ready, it means the driver will use the
default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook
too.
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-25 1:52 ` Huang Shijie
0 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-25 1:52 UTC (permalink / raw)
To: Marek Vasut
Cc: ggrahammoore, Graham Moore, Sascha Hauer, Jingoo Han,
Geert Uytterhoeven, linux-kernel, Yves Vandervennet, linux-mtd,
Artem Bityutskiy, Alan Tull, Sourav Poddar, Brian Norris,
Insop Song, David Woodhouse, Dinh Nguyen
On Fri, Apr 25, 2014 at 04:42:33AM +0200, Marek Vasut wrote:
> On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> > On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > > 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 n25q512ax3
> > > and n25q00 Micron QSPI flash chips.
> > >
> > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > ---
> > > V3:
> > > Rebase to l2-mtd spinor branch.
> > > V2:
> > > Remove leading underscore in function names.
> > > Remove type cast in dev_err call and use the proper format
> > > specifier instead.
> > > ---
> > >
> > > drivers/mtd/spi-nor/spi-nor.c | 51
> > > +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h
> > > | 4 ++++
> > > 2 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> > >
> > > }
> > >
> > > /*
> > >
> > > + * 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 spi_nor *nor)
> > > +{
> > > + int ret;
> > > + u8 val;
> > > +
> > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > > + if (ret < 0) {
> > > + pr_err("error %d reading FSR\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return val;
> > > +}
> > > +
> > > +/*
> > >
> > > * Read configuration register, returning its value in the
> > > * location. Return the configuration register value.
> > > * Returns negative if error occured.
> > >
> > > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
> > > *nor)
> > >
> > > return -ETIMEDOUT;
> > >
> > > }
> > >
> > > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > > +{
> > > + unsigned long deadline;
> > > + int sr;
> > > + int fsr;
> > > +
> > > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > > +
> > > + do {
> > > + cond_resched();
> > > +
> > > + sr = read_sr(nor);
> > > + if (sr < 0)
> > > + break;
> > > + else if (!(sr & SR_WIP)) {
> > > + fsr = read_fsr(nor);
> > > + if (fsr < 0)
> > > + break;
> > > + if (fsr & FSR_READY)
> > > + return 0;
> > > + }
> > > + } while (!time_after_eq(jiffies, deadline));
> > > +
> > > + return -ETIMEDOUT;
> > > +}
> > > +
> > >
> > > /*
> > >
> > > * Service routine to read status register until ready, or timeout
> > > occurs. * Returns non-zero if error.
> > >
> > > @@ -402,6 +447,7 @@ struct flash_info {
> > >
> > > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC works
> uniformly */
> > > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */
> > > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */
> > >
> > > +#define USE_FSR 0x80 /* use flag status register */
> > >
> > > };
> > >
> > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > >
> > > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> > >
> > > + { "n25q512ax3", 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) },
> > >
> > > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
> > > spi_device_id *id,
> > >
> > > else
> > >
> > > mtd->_write = spi_nor_write;
> > >
> > > + if (info->flags & USE_FSR)
> > > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > +
> >
> > the drivers may fills this hook itself, so the code should like this:
> > --------------------------------------------------
> > if ((info->flags & USE_FSR) &&
> > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > --------------------------------------------------
>
> I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver
> compete for a function pointer here ? I guess one should have precedence in some
> way then ... and also, they should be two different pointers, where the
> subsystem decides which to use.
the subsystem do not decides which one to use, the driver decides which one to use.
If driver has its own @wait_till_ready , it means the driver knows the feature,
and has implemented it in its own @wait_till_ready.
If the driver does not fill any wait_till_ready, it means the driver will use the
default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook
too.
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-25 1:52 ` Huang Shijie
@ 2014-04-25 22:12 ` Marek Vasut
-1 siblings, 0 replies; 40+ messages in thread
From: Marek Vasut @ 2014-04-25 22:12 UTC (permalink / raw)
To: Huang Shijie
Cc: Graham Moore, ggrahammoore, Geert Uytterhoeven, Artem Bityutskiy,
Sascha Hauer, Jingoo Han, linux-kernel, Yves Vandervennet,
linux-mtd, Insop Song, Alan Tull, Sourav Poddar, Brian Norris,
David Woodhouse, Dinh Nguyen
On Friday, April 25, 2014 at 03:52:46 AM, Huang Shijie wrote:
> On Fri, Apr 25, 2014 at 04:42:33AM +0200, Marek Vasut wrote:
> > On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> > > On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > > > 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
> > > > n25q512ax3 and n25q00 Micron QSPI flash chips.
> > > >
> > > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > > ---
> > > > V3:
> > > > Rebase to l2-mtd spinor branch.
> > > > V2:
> > > > Remove leading underscore in function names.
> > > > Remove type cast in dev_err call and use the proper format
> > > > specifier instead.
> > > > ---
> > > >
> > > > drivers/mtd/spi-nor/spi-nor.c | 51
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/mtd/spi-nor.h
> > > >
> > > > | 4 ++++
> > > >
> > > > 2 files changed, 55 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> > > >
> > > > }
> > > >
> > > > /*
> > > >
> > > > + * 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 spi_nor *nor)
> > > > +{
> > > > + int ret;
> > > > + u8 val;
> > > > +
> > > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > > > + if (ret < 0) {
> > > > + pr_err("error %d reading FSR\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return val;
> > > > +}
> > > > +
> > > > +/*
> > > >
> > > > * Read configuration register, returning its value in the
> > > > * location. Return the configuration register value.
> > > > * Returns negative if error occured.
> > > >
> > > > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct
> > > > spi_nor *nor)
> > > >
> > > > return -ETIMEDOUT;
> > > >
> > > > }
> > > >
> > > > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > > > +{
> > > > + unsigned long deadline;
> > > > + int sr;
> > > > + int fsr;
> > > > +
> > > > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > > > +
> > > > + do {
> > > > + cond_resched();
> > > > +
> > > > + sr = read_sr(nor);
> > > > + if (sr < 0)
> > > > + break;
> > > > + else if (!(sr & SR_WIP)) {
> > > > + fsr = read_fsr(nor);
> > > > + if (fsr < 0)
> > > > + break;
> > > > + if (fsr & FSR_READY)
> > > > + return 0;
> > > > + }
> > > > + } while (!time_after_eq(jiffies, deadline));
> > > > +
> > > > + return -ETIMEDOUT;
> > > > +}
> > > > +
> > > >
> > > > /*
> > > >
> > > > * Service routine to read status register until ready, or timeout
> > > > occurs. * Returns non-zero if error.
> > > >
> > > > @@ -402,6 +447,7 @@ struct flash_info {
> > > >
> > > > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC
works
> >
> > uniformly */
> >
> > > > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual
Read */
> > > > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad
Read */
> > > >
> > > > +#define USE_FSR 0x80 /* use flag status
register */
> > > >
> > > > };
> > > >
> > > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > > >
> > > > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> > > >
> > > > + { "n25q512ax3", 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)
},
> > > >
> > > > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const
> > > > struct spi_device_id *id,
> > > >
> > > > else
> > > >
> > > > mtd->_write = spi_nor_write;
> > > >
> > > > + if (info->flags & USE_FSR)
> > > > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > +
> > >
> > > the drivers may fills this hook itself, so the code should like this:
> > > --------------------------------------------------
> > >
> > > if ((info->flags & USE_FSR) &&
> > >
> > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > >
> > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > >
> > > --------------------------------------------------
> >
> > I sense a misdesign of the SPI NOR subsystem here. The subsystem and the
> > driver compete for a function pointer here ? I guess one should have
> > precedence in some way then ... and also, they should be two different
> > pointers, where the subsystem decides which to use.
>
> the subsystem do not decides which one to use, the driver decides which one
> to use.
>
> If driver has its own @wait_till_ready , it means the driver knows the
> feature, and has implemented it in its own @wait_till_ready.
>
> If the driver does not fill any wait_till_ready, it means the driver will
> use the default @wait_till_ready. We can treat the
> spi_nor_wait_till_fsr_ready as a default hook too.
I see the driver overwriting a hook previously set by the subsystem. This
doesn't seem right to me, but please correct me if I'm wrong ...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-25 22:12 ` Marek Vasut
0 siblings, 0 replies; 40+ messages in thread
From: Marek Vasut @ 2014-04-25 22:12 UTC (permalink / raw)
To: Huang Shijie
Cc: ggrahammoore, Graham Moore, Sascha Hauer, Jingoo Han,
Geert Uytterhoeven, linux-kernel, Yves Vandervennet, linux-mtd,
Artem Bityutskiy, Alan Tull, Sourav Poddar, Brian Norris,
Insop Song, David Woodhouse, Dinh Nguyen
On Friday, April 25, 2014 at 03:52:46 AM, Huang Shijie wrote:
> On Fri, Apr 25, 2014 at 04:42:33AM +0200, Marek Vasut wrote:
> > On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> > > On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > > > 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
> > > > n25q512ax3 and n25q00 Micron QSPI flash chips.
> > > >
> > > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > > ---
> > > > V3:
> > > > Rebase to l2-mtd spinor branch.
> > > > V2:
> > > > Remove leading underscore in function names.
> > > > Remove type cast in dev_err call and use the proper format
> > > > specifier instead.
> > > > ---
> > > >
> > > > drivers/mtd/spi-nor/spi-nor.c | 51
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/mtd/spi-nor.h
> > > >
> > > > | 4 ++++
> > > >
> > > > 2 files changed, 55 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> > > >
> > > > }
> > > >
> > > > /*
> > > >
> > > > + * 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 spi_nor *nor)
> > > > +{
> > > > + int ret;
> > > > + u8 val;
> > > > +
> > > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > > > + if (ret < 0) {
> > > > + pr_err("error %d reading FSR\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return val;
> > > > +}
> > > > +
> > > > +/*
> > > >
> > > > * Read configuration register, returning its value in the
> > > > * location. Return the configuration register value.
> > > > * Returns negative if error occured.
> > > >
> > > > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct
> > > > spi_nor *nor)
> > > >
> > > > return -ETIMEDOUT;
> > > >
> > > > }
> > > >
> > > > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > > > +{
> > > > + unsigned long deadline;
> > > > + int sr;
> > > > + int fsr;
> > > > +
> > > > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > > > +
> > > > + do {
> > > > + cond_resched();
> > > > +
> > > > + sr = read_sr(nor);
> > > > + if (sr < 0)
> > > > + break;
> > > > + else if (!(sr & SR_WIP)) {
> > > > + fsr = read_fsr(nor);
> > > > + if (fsr < 0)
> > > > + break;
> > > > + if (fsr & FSR_READY)
> > > > + return 0;
> > > > + }
> > > > + } while (!time_after_eq(jiffies, deadline));
> > > > +
> > > > + return -ETIMEDOUT;
> > > > +}
> > > > +
> > > >
> > > > /*
> > > >
> > > > * Service routine to read status register until ready, or timeout
> > > > occurs. * Returns non-zero if error.
> > > >
> > > > @@ -402,6 +447,7 @@ struct flash_info {
> > > >
> > > > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC
works
> >
> > uniformly */
> >
> > > > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual
Read */
> > > > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad
Read */
> > > >
> > > > +#define USE_FSR 0x80 /* use flag status
register */
> > > >
> > > > };
> > > >
> > > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > > >
> > > > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> > > >
> > > > + { "n25q512ax3", 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)
},
> > > >
> > > > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const
> > > > struct spi_device_id *id,
> > > >
> > > > else
> > > >
> > > > mtd->_write = spi_nor_write;
> > > >
> > > > + if (info->flags & USE_FSR)
> > > > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > +
> > >
> > > the drivers may fills this hook itself, so the code should like this:
> > > --------------------------------------------------
> > >
> > > if ((info->flags & USE_FSR) &&
> > >
> > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > >
> > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > >
> > > --------------------------------------------------
> >
> > I sense a misdesign of the SPI NOR subsystem here. The subsystem and the
> > driver compete for a function pointer here ? I guess one should have
> > precedence in some way then ... and also, they should be two different
> > pointers, where the subsystem decides which to use.
>
> the subsystem do not decides which one to use, the driver decides which one
> to use.
>
> If driver has its own @wait_till_ready , it means the driver knows the
> feature, and has implemented it in its own @wait_till_ready.
>
> If the driver does not fill any wait_till_ready, it means the driver will
> use the default @wait_till_ready. We can treat the
> spi_nor_wait_till_fsr_ready as a default hook too.
I see the driver overwriting a hook previously set by the subsystem. This
doesn't seem right to me, but please correct me if I'm wrong ...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-25 22:12 ` Marek Vasut
@ 2014-04-26 3:10 ` Huang Shijie
-1 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-26 3:10 UTC (permalink / raw)
To: Marek Vasut
Cc: Huang Shijie, Graham Moore, ggrahammoore, Geert Uytterhoeven,
Artem Bityutskiy, Sascha Hauer, Jingoo Han, linux-kernel,
Yves Vandervennet, linux-mtd, Insop Song, Alan Tull,
Sourav Poddar, Brian Norris, David Woodhouse, Dinh Nguyen
On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > the drivers may fills this hook itself, so the code should like this:
> > > > --------------------------------------------------
> > > >
> > > > if ((info->flags & USE_FSR) &&
> > > >
> > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > >
> > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > >
> > > > --------------------------------------------------
> > >
> > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and the
> > > driver compete for a function pointer here ? I guess one should have
> > > precedence in some way then ... and also, they should be two different
> > > pointers, where the subsystem decides which to use.
> >
> > the subsystem do not decides which one to use, the driver decides which one
> > to use.
> >
> > If driver has its own @wait_till_ready , it means the driver knows the
> > feature, and has implemented it in its own @wait_till_ready.
> >
> > If the driver does not fill any wait_till_ready, it means the driver will
> > use the default @wait_till_ready. We can treat the
> > spi_nor_wait_till_fsr_ready as a default hook too.
>
> I see the driver overwriting a hook previously set by the subsystem. This
not sure ;)
The driver set the hooks before the subsystem set these hooks.
If the driver has already set the @wait_till_ready hook before it calls
the spi_nor_scan, the subsystem will not set the hook anymore.
Please see the spi_nor_check().
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-26 3:10 ` Huang Shijie
0 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-26 3:10 UTC (permalink / raw)
To: Marek Vasut
Cc: ggrahammoore, Yves Vandervennet, Artem Bityutskiy, Sascha Hauer,
Jingoo Han, Geert Uytterhoeven, linux-kernel, Huang Shijie,
linux-mtd, Graham Moore, Alan Tull, Sourav Poddar, Brian Norris,
Insop Song, David Woodhouse, Dinh Nguyen
On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > the drivers may fills this hook itself, so the code should like this:
> > > > --------------------------------------------------
> > > >
> > > > if ((info->flags & USE_FSR) &&
> > > >
> > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > >
> > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > >
> > > > --------------------------------------------------
> > >
> > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and the
> > > driver compete for a function pointer here ? I guess one should have
> > > precedence in some way then ... and also, they should be two different
> > > pointers, where the subsystem decides which to use.
> >
> > the subsystem do not decides which one to use, the driver decides which one
> > to use.
> >
> > If driver has its own @wait_till_ready , it means the driver knows the
> > feature, and has implemented it in its own @wait_till_ready.
> >
> > If the driver does not fill any wait_till_ready, it means the driver will
> > use the default @wait_till_ready. We can treat the
> > spi_nor_wait_till_fsr_ready as a default hook too.
>
> I see the driver overwriting a hook previously set by the subsystem. This
not sure ;)
The driver set the hooks before the subsystem set these hooks.
If the driver has already set the @wait_till_ready hook before it calls
the spi_nor_scan, the subsystem will not set the hook anymore.
Please see the spi_nor_check().
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-26 3:10 ` Huang Shijie
@ 2014-04-28 5:06 ` Marek Vasut
-1 siblings, 0 replies; 40+ messages in thread
From: Marek Vasut @ 2014-04-28 5:06 UTC (permalink / raw)
To: Huang Shijie
Cc: Huang Shijie, Graham Moore, ggrahammoore, Geert Uytterhoeven,
Artem Bityutskiy, Sascha Hauer, Jingoo Han, linux-kernel,
Yves Vandervennet, linux-mtd, Insop Song, Alan Tull,
Sourav Poddar, Brian Norris, David Woodhouse, Dinh Nguyen
On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote:
> On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > > the drivers may fills this hook itself, so the code should like this:
> > > > > --------------------------------------------------
> > > > >
> > > > > if ((info->flags & USE_FSR) &&
> > > > >
> > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > > >
> > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > >
> > > > > --------------------------------------------------
> > > >
> > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and
> > > > the driver compete for a function pointer here ? I guess one should
> > > > have precedence in some way then ... and also, they should be two
> > > > different pointers, where the subsystem decides which to use.
> > >
> > > the subsystem do not decides which one to use, the driver decides which
> > > one to use.
> > >
> > > If driver has its own @wait_till_ready , it means the driver knows the
> > > feature, and has implemented it in its own @wait_till_ready.
> > >
> > > If the driver does not fill any wait_till_ready, it means the driver
> > > will use the default @wait_till_ready. We can treat the
> > > spi_nor_wait_till_fsr_ready as a default hook too.
> >
> > I see the driver overwriting a hook previously set by the subsystem. This
>
> not sure ;)
>
> The driver set the hooks before the subsystem set these hooks.
>
> If the driver has already set the @wait_till_ready hook before it calls
> the spi_nor_scan, the subsystem will not set the hook anymore.
>
> Please see the spi_nor_check().
Two things competing over the same pointer looks misdesigned to me. I will need
to dig into this one more time ...
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-28 5:06 ` Marek Vasut
0 siblings, 0 replies; 40+ messages in thread
From: Marek Vasut @ 2014-04-28 5:06 UTC (permalink / raw)
To: Huang Shijie
Cc: ggrahammoore, Yves Vandervennet, Artem Bityutskiy, Sascha Hauer,
Jingoo Han, Geert Uytterhoeven, linux-kernel, Huang Shijie,
linux-mtd, Graham Moore, Alan Tull, Sourav Poddar, Brian Norris,
Insop Song, David Woodhouse, Dinh Nguyen
On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote:
> On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > > the drivers may fills this hook itself, so the code should like this:
> > > > > --------------------------------------------------
> > > > >
> > > > > if ((info->flags & USE_FSR) &&
> > > > >
> > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > > >
> > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > >
> > > > > --------------------------------------------------
> > > >
> > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and
> > > > the driver compete for a function pointer here ? I guess one should
> > > > have precedence in some way then ... and also, they should be two
> > > > different pointers, where the subsystem decides which to use.
> > >
> > > the subsystem do not decides which one to use, the driver decides which
> > > one to use.
> > >
> > > If driver has its own @wait_till_ready , it means the driver knows the
> > > feature, and has implemented it in its own @wait_till_ready.
> > >
> > > If the driver does not fill any wait_till_ready, it means the driver
> > > will use the default @wait_till_ready. We can treat the
> > > spi_nor_wait_till_fsr_ready as a default hook too.
> >
> > I see the driver overwriting a hook previously set by the subsystem. This
>
> not sure ;)
>
> The driver set the hooks before the subsystem set these hooks.
>
> If the driver has already set the @wait_till_ready hook before it calls
> the spi_nor_scan, the subsystem will not set the hook anymore.
>
> Please see the spi_nor_check().
Two things competing over the same pointer looks misdesigned to me. I will need
to dig into this one more time ...
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-28 5:06 ` Marek Vasut
@ 2014-04-28 7:06 ` Huang Shijie
-1 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-28 7:06 UTC (permalink / raw)
To: Marek Vasut
Cc: Huang Shijie, Graham Moore, ggrahammoore, Geert Uytterhoeven,
Artem Bityutskiy, Sascha Hauer, Jingoo Han, linux-kernel,
Yves Vandervennet, linux-mtd, Insop Song, Alan Tull,
Sourav Poddar, Brian Norris, David Woodhouse, Dinh Nguyen
On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
> On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote:
> > On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > > > the drivers may fills this hook itself, so the code should like this:
> > > > > > --------------------------------------------------
> > > > > >
> > > > > > if ((info->flags & USE_FSR) &&
> > > > > >
> > > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > > > >
> > > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > > >
> > > > > > --------------------------------------------------
> > > > >
> > > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and
> > > > > the driver compete for a function pointer here ? I guess one should
> > > > > have precedence in some way then ... and also, they should be two
> > > > > different pointers, where the subsystem decides which to use.
> > > >
> > > > the subsystem do not decides which one to use, the driver decides which
> > > > one to use.
> > > >
> > > > If driver has its own @wait_till_ready , it means the driver knows the
> > > > feature, and has implemented it in its own @wait_till_ready.
> > > >
> > > > If the driver does not fill any wait_till_ready, it means the driver
> > > > will use the default @wait_till_ready. We can treat the
> > > > spi_nor_wait_till_fsr_ready as a default hook too.
> > >
> > > I see the driver overwriting a hook previously set by the subsystem. This
> >
> > not sure ;)
> >
> > The driver set the hooks before the subsystem set these hooks.
> >
> > If the driver has already set the @wait_till_ready hook before it calls
> > the spi_nor_scan, the subsystem will not set the hook anymore.
> >
> > Please see the spi_nor_check().
>
> Two things competing over the same pointer looks misdesigned to me. I will need
> to dig into this one more time ...
Please refer to the code for NAND chip, the nand_get_flash_type() :
-----------------------------------------------------------
/* Do not replace user supplied command function! */
if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
chip->cmdfunc = nand_command_lp;
-----------------------------------------------------------
It uses the same logic:
" Do not replace user supplied command function!"
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-28 7:06 ` Huang Shijie
0 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-28 7:06 UTC (permalink / raw)
To: Marek Vasut
Cc: ggrahammoore, Insop Song, Graham Moore, Sascha Hauer, Jingoo Han,
Geert Uytterhoeven, linux-kernel, Yves Vandervennet, linux-mtd,
Artem Bityutskiy, Alan Tull, Sourav Poddar, Brian Norris,
Huang Shijie, David Woodhouse, Dinh Nguyen
On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
> On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote:
> > On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > > > the drivers may fills this hook itself, so the code should like this:
> > > > > > --------------------------------------------------
> > > > > >
> > > > > > if ((info->flags & USE_FSR) &&
> > > > > >
> > > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > > > >
> > > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > > >
> > > > > > --------------------------------------------------
> > > > >
> > > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and
> > > > > the driver compete for a function pointer here ? I guess one should
> > > > > have precedence in some way then ... and also, they should be two
> > > > > different pointers, where the subsystem decides which to use.
> > > >
> > > > the subsystem do not decides which one to use, the driver decides which
> > > > one to use.
> > > >
> > > > If driver has its own @wait_till_ready , it means the driver knows the
> > > > feature, and has implemented it in its own @wait_till_ready.
> > > >
> > > > If the driver does not fill any wait_till_ready, it means the driver
> > > > will use the default @wait_till_ready. We can treat the
> > > > spi_nor_wait_till_fsr_ready as a default hook too.
> > >
> > > I see the driver overwriting a hook previously set by the subsystem. This
> >
> > not sure ;)
> >
> > The driver set the hooks before the subsystem set these hooks.
> >
> > If the driver has already set the @wait_till_ready hook before it calls
> > the spi_nor_scan, the subsystem will not set the hook anymore.
> >
> > Please see the spi_nor_check().
>
> Two things competing over the same pointer looks misdesigned to me. I will need
> to dig into this one more time ...
Please refer to the code for NAND chip, the nand_get_flash_type() :
-----------------------------------------------------------
/* Do not replace user supplied command function! */
if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
chip->cmdfunc = nand_command_lp;
-----------------------------------------------------------
It uses the same logic:
" Do not replace user supplied command function!"
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-28 7:06 ` Huang Shijie
@ 2014-04-28 14:22 ` Graham Moore
-1 siblings, 0 replies; 40+ messages in thread
From: Graham Moore @ 2014-04-28 14:22 UTC (permalink / raw)
To: Huang Shijie
Cc: Marek Vasut, Huang Shijie, Graham Moore, Geert Uytterhoeven,
Artem Bityutskiy, Sascha Hauer, Jingoo Han, linux-kernel,
Yves Vandervennet, linux-mtd, Insop Song, Alan Tull,
Sourav Poddar, Brian Norris, David Woodhouse, Dinh Nguyen
On Mon, Apr 28, 2014 at 2:06 AM, Huang Shijie <b32955@freescale.com> wrote:
> On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
>>
>> Two things competing over the same pointer looks misdesigned to me. I will need
>> to dig into this one more time ...
> Please refer to the code for NAND chip, the nand_get_flash_type() :
>
> -----------------------------------------------------------
> /* Do not replace user supplied command function! */
> if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> chip->cmdfunc = nand_command_lp;
> -----------------------------------------------------------
>
> It uses the same logic:
> " Do not replace user supplied command function!"
>
> thanks
> Huang Shijie
I would like to set the @wait_till_ready in m25p80.c, because the
USE_FSR flag is only for Micron chips. But the m25p80 driver doesn't
have access to the flags, they are contained in spi-nor.c. It seems
to me that the driver_data should be accessible by the driver. So
maybe there is room for improvement there.
Thanks,
Graham Moore
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-28 14:22 ` Graham Moore
0 siblings, 0 replies; 40+ messages in thread
From: Graham Moore @ 2014-04-28 14:22 UTC (permalink / raw)
To: Huang Shijie
Cc: Marek Vasut, Geert Uytterhoeven, Insop Song, Graham Moore,
Sascha Hauer, Jingoo Han, linux-kernel, Yves Vandervennet,
linux-mtd, Artem Bityutskiy, Alan Tull, Sourav Poddar,
Brian Norris, Huang Shijie, David Woodhouse, Dinh Nguyen
On Mon, Apr 28, 2014 at 2:06 AM, Huang Shijie <b32955@freescale.com> wrote:
> On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
>>
>> Two things competing over the same pointer looks misdesigned to me. I will need
>> to dig into this one more time ...
> Please refer to the code for NAND chip, the nand_get_flash_type() :
>
> -----------------------------------------------------------
> /* Do not replace user supplied command function! */
> if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> chip->cmdfunc = nand_command_lp;
> -----------------------------------------------------------
>
> It uses the same logic:
> " Do not replace user supplied command function!"
>
> thanks
> Huang Shijie
I would like to set the @wait_till_ready in m25p80.c, because the
USE_FSR flag is only for Micron chips. But the m25p80 driver doesn't
have access to the flags, they are contained in spi-nor.c. It seems
to me that the driver_data should be accessible by the driver. So
maybe there is room for improvement there.
Thanks,
Graham Moore
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-28 14:22 ` Graham Moore
@ 2014-04-28 15:37 ` Huang Shijie
-1 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-28 15:37 UTC (permalink / raw)
To: Graham Moore
Cc: Huang Shijie, Marek Vasut, Graham Moore, Geert Uytterhoeven,
Artem Bityutskiy, Sascha Hauer, Jingoo Han, linux-kernel,
Yves Vandervennet, linux-mtd, Insop Song, Alan Tull,
Sourav Poddar, Brian Norris, David Woodhouse, Dinh Nguyen
On Mon, Apr 28, 2014 at 09:22:58AM -0500, Graham Moore wrote:
> On Mon, Apr 28, 2014 at 2:06 AM, Huang Shijie <b32955@freescale.com> wrote:
> > On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
> >>
> >> Two things competing over the same pointer looks misdesigned to me. I will need
> >> to dig into this one more time ...
> > Please refer to the code for NAND chip, the nand_get_flash_type() :
> >
> > -----------------------------------------------------------
> > /* Do not replace user supplied command function! */
> > if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> > chip->cmdfunc = nand_command_lp;
> > -----------------------------------------------------------
> >
> > It uses the same logic:
> > " Do not replace user supplied command function!"
> >
> > thanks
> > Huang Shijie
>
> I would like to set the @wait_till_ready in m25p80.c, because the
> USE_FSR flag is only for Micron chips. But the m25p80 driver doesn't
m25p80.c is not the right place.
we should put these code in the spi-nor.c
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-28 15:37 ` Huang Shijie
0 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-28 15:37 UTC (permalink / raw)
To: Graham Moore
Cc: Marek Vasut, Geert Uytterhoeven, Yves Vandervennet,
Artem Bityutskiy, Sascha Hauer, Jingoo Han, linux-kernel,
Huang Shijie, linux-mtd, Graham Moore, Alan Tull, Sourav Poddar,
Brian Norris, Insop Song, David Woodhouse, Dinh Nguyen
On Mon, Apr 28, 2014 at 09:22:58AM -0500, Graham Moore wrote:
> On Mon, Apr 28, 2014 at 2:06 AM, Huang Shijie <b32955@freescale.com> wrote:
> > On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
> >>
> >> Two things competing over the same pointer looks misdesigned to me. I will need
> >> to dig into this one more time ...
> > Please refer to the code for NAND chip, the nand_get_flash_type() :
> >
> > -----------------------------------------------------------
> > /* Do not replace user supplied command function! */
> > if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
> > chip->cmdfunc = nand_command_lp;
> > -----------------------------------------------------------
> >
> > It uses the same logic:
> > " Do not replace user supplied command function!"
> >
> > thanks
> > Huang Shijie
>
> I would like to set the @wait_till_ready in m25p80.c, because the
> USE_FSR flag is only for Micron chips. But the m25p80 driver doesn't
m25p80.c is not the right place.
we should put these code in the spi-nor.c
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-28 5:06 ` Marek Vasut
@ 2014-07-12 2:07 ` Brian Norris
-1 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2014-07-12 2:07 UTC (permalink / raw)
To: Marek Vasut
Cc: Huang Shijie, Huang Shijie, Graham Moore, ggrahammoore,
Geert Uytterhoeven, Artem Bityutskiy, Sascha Hauer, Jingoo Han,
linux-kernel, Yves Vandervennet, linux-mtd, Insop Song,
Alan Tull, Sourav Poddar, David Woodhouse, Dinh Nguyen
Hi guys,
Sorry to revisit this way late, and sorry for not paying as much
attention initially. I'm prepped to merge v4, but some of the
conversation matches what I was just thinking.
On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
> On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote:
> > On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > > > the drivers may fills this hook itself, so the code should like this:
> > > > > > --------------------------------------------------
> > > > > >
> > > > > > if ((info->flags & USE_FSR) &&
> > > > > >
> > > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > > > >
> > > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > > >
> > > > > > --------------------------------------------------
> > > > >
> > > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and
> > > > > the driver compete for a function pointer here ? I guess one should
> > > > > have precedence in some way then ... and also, they should be two
> > > > > different pointers, where the subsystem decides which to use.
> > > >
> > > > the subsystem do not decides which one to use, the driver decides which
> > > > one to use.
> > > >
> > > > If driver has its own @wait_till_ready , it means the driver knows the
> > > > feature, and has implemented it in its own @wait_till_ready.
> > > >
> > > > If the driver does not fill any wait_till_ready, it means the driver
> > > > will use the default @wait_till_ready. We can treat the
> > > > spi_nor_wait_till_fsr_ready as a default hook too.
> > >
> > > I see the driver overwriting a hook previously set by the subsystem. This
> >
> > not sure ;)
> >
> > The driver set the hooks before the subsystem set these hooks.
> >
> > If the driver has already set the @wait_till_ready hook before it calls
> > the spi_nor_scan, the subsystem will not set the hook anymore.
> >
> > Please see the spi_nor_check().
>
> Two things competing over the same pointer looks misdesigned to me. I will need
> to dig into this one more time ...
Yes, that is misdesigned. And looking at nand_base for examples is not
foolproof; it has quite a bit of legacy and workarounds. It'd be best to
get the design right for spi-nor.
The subsystem code should not require a function pointer for FSR vs.
non-FSR -- all devices should be able to use the same function. We just
need to stash some flash-ID-related data (i.e., a flags field) in the
spi_nor struct. On the plus side, we can avoid the code duplication
between spi_nor_wait_till_fsr_ready() and spi_nor_wait_till_ready().
I think the wait_till_ready pointer should be reserved for the driver,
as a hardware-specific "wait" function.
This still leaves the question of whether the SPI NOR core should assume
that any driver's 'wait_till_ready' function (if present) actually
implements all necessary waits (FSR vs. non-FSR, for instance). I'd
argue that's a maintenance burden, and that the subsystem should still
do a sanity check that the status register is correct. After all, that's
what the ->{read,write}_reg() functions are useful for. But perhaps
there is some performance argument for avoiding the (potentially
redundant) register checks?
Anyway, I've tested v4, and I plan to merge it soon. Patches can be sent
on top. (I may even cook up my own.)
Regards,
Brian
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-07-12 2:07 ` Brian Norris
0 siblings, 0 replies; 40+ messages in thread
From: Brian Norris @ 2014-07-12 2:07 UTC (permalink / raw)
To: Marek Vasut
Cc: ggrahammoore, Insop Song, Yves Vandervennet, Artem Bityutskiy,
Sascha Hauer, Jingoo Han, Geert Uytterhoeven, linux-kernel,
Huang Shijie, linux-mtd, Graham Moore, Alan Tull, Sourav Poddar,
Huang Shijie, David Woodhouse, Dinh Nguyen
Hi guys,
Sorry to revisit this way late, and sorry for not paying as much
attention initially. I'm prepped to merge v4, but some of the
conversation matches what I was just thinking.
On Mon, Apr 28, 2014 at 07:06:17AM +0200, Marek Vasut wrote:
> On Saturday, April 26, 2014 at 05:10:13 AM, Huang Shijie wrote:
> > On Sat, Apr 26, 2014 at 12:12:24AM +0200, Marek Vasut wrote:
> > > > > > the drivers may fills this hook itself, so the code should like this:
> > > > > > --------------------------------------------------
> > > > > >
> > > > > > if ((info->flags & USE_FSR) &&
> > > > > >
> > > > > > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > > > > >
> > > > > > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > > > >
> > > > > > --------------------------------------------------
> > > > >
> > > > > I sense a misdesign of the SPI NOR subsystem here. The subsystem and
> > > > > the driver compete for a function pointer here ? I guess one should
> > > > > have precedence in some way then ... and also, they should be two
> > > > > different pointers, where the subsystem decides which to use.
> > > >
> > > > the subsystem do not decides which one to use, the driver decides which
> > > > one to use.
> > > >
> > > > If driver has its own @wait_till_ready , it means the driver knows the
> > > > feature, and has implemented it in its own @wait_till_ready.
> > > >
> > > > If the driver does not fill any wait_till_ready, it means the driver
> > > > will use the default @wait_till_ready. We can treat the
> > > > spi_nor_wait_till_fsr_ready as a default hook too.
> > >
> > > I see the driver overwriting a hook previously set by the subsystem. This
> >
> > not sure ;)
> >
> > The driver set the hooks before the subsystem set these hooks.
> >
> > If the driver has already set the @wait_till_ready hook before it calls
> > the spi_nor_scan, the subsystem will not set the hook anymore.
> >
> > Please see the spi_nor_check().
>
> Two things competing over the same pointer looks misdesigned to me. I will need
> to dig into this one more time ...
Yes, that is misdesigned. And looking at nand_base for examples is not
foolproof; it has quite a bit of legacy and workarounds. It'd be best to
get the design right for spi-nor.
The subsystem code should not require a function pointer for FSR vs.
non-FSR -- all devices should be able to use the same function. We just
need to stash some flash-ID-related data (i.e., a flags field) in the
spi_nor struct. On the plus side, we can avoid the code duplication
between spi_nor_wait_till_fsr_ready() and spi_nor_wait_till_ready().
I think the wait_till_ready pointer should be reserved for the driver,
as a hardware-specific "wait" function.
This still leaves the question of whether the SPI NOR core should assume
that any driver's 'wait_till_ready' function (if present) actually
implements all necessary waits (FSR vs. non-FSR, for instance). I'd
argue that's a maintenance burden, and that the subsystem should still
do a sanity check that the status register is correct. After all, that's
what the ->{read,write}_reg() functions are useful for. But perhaps
there is some performance argument for avoiding the (potentially
redundant) register checks?
Anyway, I've tested v4, and I plan to merge it soon. Patches can be sent
on top. (I may even cook up my own.)
Regards,
Brian
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH V3] Add support for flag status register on Micron chips.
2014-04-25 2:42 ` Marek Vasut
@ 2014-04-25 1:54 ` Huang Shijie
-1 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-25 1:54 UTC (permalink / raw)
To: Marek Vasut
Cc: Graham Moore, ggrahammoore, Geert Uytterhoeven, Artem Bityutskiy,
Sascha Hauer, Jingoo Han, linux-kernel, Yves Vandervennet,
linux-mtd, Insop Song, Alan Tull, Sourav Poddar, Brian Norris,
David Woodhouse, Dinh Nguyen
On Fri, Apr 25, 2014 at 04:42:33AM +0200, Marek Vasut wrote:
> On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> > On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > > 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 n25q512ax3
> > > and n25q00 Micron QSPI flash chips.
> > >
> > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > ---
> > > V3:
> > > Rebase to l2-mtd spinor branch.
> > > V2:
> > > Remove leading underscore in function names.
> > > Remove type cast in dev_err call and use the proper format
> > > specifier instead.
> > > ---
> > >
> > > drivers/mtd/spi-nor/spi-nor.c | 51
> > > +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h
> > > | 4 ++++
> > > 2 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> > >
> > > }
> > >
> > > /*
> > >
> > > + * 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 spi_nor *nor)
> > > +{
> > > + int ret;
> > > + u8 val;
> > > +
> > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > > + if (ret < 0) {
> > > + pr_err("error %d reading FSR\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return val;
> > > +}
> > > +
> > > +/*
> > >
> > > * Read configuration register, returning its value in the
> > > * location. Return the configuration register value.
> > > * Returns negative if error occured.
> > >
> > > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
> > > *nor)
> > >
> > > return -ETIMEDOUT;
> > >
> > > }
> > >
> > > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > > +{
> > > + unsigned long deadline;
> > > + int sr;
> > > + int fsr;
> > > +
> > > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > > +
> > > + do {
> > > + cond_resched();
> > > +
> > > + sr = read_sr(nor);
> > > + if (sr < 0)
> > > + break;
> > > + else if (!(sr & SR_WIP)) {
> > > + fsr = read_fsr(nor);
> > > + if (fsr < 0)
> > > + break;
> > > + if (fsr & FSR_READY)
> > > + return 0;
> > > + }
> > > + } while (!time_after_eq(jiffies, deadline));
> > > +
> > > + return -ETIMEDOUT;
> > > +}
> > > +
> > >
> > > /*
> > >
> > > * Service routine to read status register until ready, or timeout
> > > occurs. * Returns non-zero if error.
> > >
> > > @@ -402,6 +447,7 @@ struct flash_info {
> > >
> > > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC works
> uniformly */
> > > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */
> > > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */
> > >
> > > +#define USE_FSR 0x80 /* use flag status register */
> > >
> > > };
> > >
> > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > >
> > > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> > >
> > > + { "n25q512ax3", 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) },
> > >
> > > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
> > > spi_device_id *id,
> > >
> > > else
> > >
> > > mtd->_write = spi_nor_write;
> > >
> > > + if (info->flags & USE_FSR)
> > > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > +
> >
> > the drivers may fills this hook itself, so the code should like this:
> > --------------------------------------------------
> > if ((info->flags & USE_FSR) &&
> > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > --------------------------------------------------
sorry, the code should like this:
--------------------------------------------------
if ((info->flags & USE_FSR) &&
nor->wait_till_ready == spi_nor_wait_till_ready)
nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
--------------------------------------------------
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH V3] Add support for flag status register on Micron chips.
@ 2014-04-25 1:54 ` Huang Shijie
0 siblings, 0 replies; 40+ messages in thread
From: Huang Shijie @ 2014-04-25 1:54 UTC (permalink / raw)
To: Marek Vasut
Cc: ggrahammoore, Graham Moore, Sascha Hauer, Jingoo Han,
Geert Uytterhoeven, linux-kernel, Yves Vandervennet, linux-mtd,
Artem Bityutskiy, Alan Tull, Sourav Poddar, Brian Norris,
Insop Song, David Woodhouse, Dinh Nguyen
On Fri, Apr 25, 2014 at 04:42:33AM +0200, Marek Vasut wrote:
> On Friday, April 25, 2014 at 03:34:36 AM, Huang Shijie wrote:
> > On Tue, Apr 22, 2014 at 09:03:16AM -0500, Graham Moore wrote:
> > > 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 n25q512ax3
> > > and n25q00 Micron QSPI flash chips.
> > >
> > > Signed-off-by: Graham Moore <grmoore@altera.com>
> > > ---
> > > V3:
> > > Rebase to l2-mtd spinor branch.
> > > V2:
> > > Remove leading underscore in function names.
> > > Remove type cast in dev_err call and use the proper format
> > > specifier instead.
> > > ---
> > >
> > > drivers/mtd/spi-nor/spi-nor.c | 51
> > > +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h
> > > | 4 ++++
> > > 2 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
> > >
> > > }
> > >
> > > /*
> > >
> > > + * 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 spi_nor *nor)
> > > +{
> > > + int ret;
> > > + u8 val;
> > > +
> > > + ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
> > > + if (ret < 0) {
> > > + pr_err("error %d reading FSR\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return val;
> > > +}
> > > +
> > > +/*
> > >
> > > * Read configuration register, returning its value in the
> > > * location. Return the configuration register value.
> > > * Returns negative if error occured.
> > >
> > > @@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
> > > *nor)
> > >
> > > return -ETIMEDOUT;
> > >
> > > }
> > >
> > > +static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
> > > +{
> > > + unsigned long deadline;
> > > + int sr;
> > > + int fsr;
> > > +
> > > + deadline = jiffies + MAX_READY_WAIT_JIFFIES;
> > > +
> > > + do {
> > > + cond_resched();
> > > +
> > > + sr = read_sr(nor);
> > > + if (sr < 0)
> > > + break;
> > > + else if (!(sr & SR_WIP)) {
> > > + fsr = read_fsr(nor);
> > > + if (fsr < 0)
> > > + break;
> > > + if (fsr & FSR_READY)
> > > + return 0;
> > > + }
> > > + } while (!time_after_eq(jiffies, deadline));
> > > +
> > > + return -ETIMEDOUT;
> > > +}
> > > +
> > >
> > > /*
> > >
> > > * Service routine to read status register until ready, or timeout
> > > occurs. * Returns non-zero if error.
> > >
> > > @@ -402,6 +447,7 @@ struct flash_info {
> > >
> > > #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC works
> uniformly */
> > > #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */
> > > #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */
> > >
> > > +#define USE_FSR 0x80 /* use flag status register */
> > >
> > > };
> > >
> > > #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> > >
> > > @@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_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) },
> > >
> > > + { "n25q512ax3", 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) },
> > >
> > > @@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
> > > spi_device_id *id,
> > >
> > > else
> > >
> > > mtd->_write = spi_nor_write;
> > >
> > > + if (info->flags & USE_FSR)
> > > + nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > > +
> >
> > the drivers may fills this hook itself, so the code should like this:
> > --------------------------------------------------
> > if ((info->flags & USE_FSR) &&
> > nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
> > nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
> > --------------------------------------------------
sorry, the code should like this:
--------------------------------------------------
if ((info->flags & USE_FSR) &&
nor->wait_till_ready == spi_nor_wait_till_ready)
nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
--------------------------------------------------
thanks
Huang Shijie
^ permalink raw reply [flat|nested] 40+ messages in thread