Patchwork [3/8] mtd: spi-nor: move "wait-till-ready" checks into erase/write functions

login
register
mail settings
Submitter Brian Norris
Date Aug. 7, 2014, 1:16 a.m.
Message ID <1407374222-8448-4-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/377703/
State Accepted
Commit dfa9c0cba4ea20e766bbb4f89152b05d00ab9ab3
Headers show

Comments

Brian Norris - Aug. 7, 2014, 1:16 a.m.
We shouldn't have *every* function checking if a previous write is
complete; this should be done synchronously after each write/erase.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)
Marek Vasut - Aug. 7, 2014, 2:24 p.m.
On Thursday, August 07, 2014 at 03:16:57 AM, Brian Norris wrote:
> We shouldn't have *every* function checking if a previous write is
> complete; this should be done synchronously after each write/erase.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Nice!

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut
Huang Shijie - Aug. 9, 2014, 8:42 a.m.
On Wed, Aug 06, 2014 at 06:16:57PM -0700, Brian Norris wrote:
> We shouldn't have *every* function checking if a previous write is
> complete; this should be done synchronously after each write/erase.

IMHO, this is not a good idea. :(
this patch prevents the erase-suspend and program-suspend.
We should add these two features for spi-nor in future.

thanks
Huang Shijie
Brian Norris - Aug. 11, 2014, 6:23 p.m.
On Sat, Aug 09, 2014 at 04:42:24PM +0800, Huang Shijie wrote:
> On Wed, Aug 06, 2014 at 06:16:57PM -0700, Brian Norris wrote:
> > We shouldn't have *every* function checking if a previous write is
> > complete; this should be done synchronously after each write/erase.
> 
> IMHO, this is not a good idea. :(

Do you mean you think it is a good idea for every unrelated function to
check if the previous erase/write is complete?

> this patch prevents the erase-suspend and program-suspend.
> We should add these two features for spi-nor in future.

OK, how would you propose that such features be implemented, and how
would they be used to the benefit of higher layers?

Directed toward the former: specifically, how does leaving the
SR/FSR-checking burden on all subsequent commands benefit a potential
erase/program suspend implementation? The code is not written at all
with erase/program suspend in mind, and the current patch solves a
current problem; that we perform checking in all the wrong places.

To the latter: are file systems (e.g., UBIFS) aware of suspend-able
program/erase? Would they have the knowledge to take advantage of
suspend-able program/erase? i.e., could they suspend an unimportant
erase command in order to prioritize a read or write?

Finally, this patch mostly prepares for elimination of code from
lower-level drivers (m25p80.c and fsl-quadspi, in the following two
patches). These drivers should *not* be worrying about the details of
command statuses; this should be handled by the generic code
(spi-nor.c).

So, unless you can provide some outline as to how program/erase suspend
can be implemented reasonably within this framework, and how this
particular patch makes that so much more difficult, I plan to move
forward with this.

Thanks,
Brian
Huang Shijie - Aug. 12, 2014, 1:37 a.m.
On Mon, Aug 11, 2014 at 11:23:04AM -0700, Brian Norris wrote:
> On Sat, Aug 09, 2014 at 04:42:24PM +0800, Huang Shijie wrote:
> > On Wed, Aug 06, 2014 at 06:16:57PM -0700, Brian Norris wrote:
> > > We shouldn't have *every* function checking if a previous write is
> > > complete; this should be done synchronously after each write/erase.
> > 
> > IMHO, this is not a good idea. :(
> 
> Do you mean you think it is a good idea for every unrelated function to
> check if the previous erase/write is complete?

i guess only the read function should need the suspend.
I does not dig this issue too.
> 
> > this patch prevents the erase-suspend and program-suspend.
> > We should add these two features for spi-nor in future.
> 
> OK, how would you propose that such features be implemented, and how
> would they be used to the benefit of higher layers?

Please refer to cfi_cmdset_0002.c. The parallel NOR has implemented this
feature. 
> 
> Directed toward the former: specifically, how does leaving the
> SR/FSR-checking burden on all subsequent commands benefit a potential
> erase/program suspend implementation? The code is not written at all
> with erase/program suspend in mind, and the current patch solves a
> current problem; that we perform checking in all the wrong places.
> 
> To the latter: are file systems (e.g., UBIFS) aware of suspend-able
> program/erase? Would they have the knowledge to take advantage of
The file systems should not know the suspend feature.

It is transparent to them. The suspend feature will make the file system
run much faster. sorry, i have forgotten how did i test this feature, it
was long time ago.


> suspend-able program/erase? i.e., could they suspend an unimportant
> erase command in order to prioritize a read or write?

they do not suspend the erase, the SPI-NOR framework should do the job.

> 
> Finally, this patch mostly prepares for elimination of code from
> lower-level drivers (m25p80.c and fsl-quadspi, in the following two
> patches). These drivers should *not* be worrying about the details of
> command statuses; this should be handled by the generic code
> (spi-nor.c).
> 
> So, unless you can provide some outline as to how program/erase suspend
> can be implemented reasonably within this framework, and how this
> particular patch makes that so much more difficult, I plan to move
> forward with this.
I do not have time to implement this feature recently. Please go
forward with your patch. I guess the one who want to add this feature
will change the code himself.


thanks
Huang Shijie

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d77c93232b76..227e2743203e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -229,15 +229,8 @@  static int wait_till_ready(struct spi_nor *nor)
  */
 static int erase_chip(struct spi_nor *nor)
 {
-	int ret;
-
 	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd->size >> 10));
 
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(nor);
-	if (ret)
-		return ret;
-
 	/* Send write enable, then erase commands. */
 	write_enable(nor);
 
@@ -300,6 +293,10 @@  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 			goto erase_err;
 		}
 
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			goto erase_err;
+
 	/* REVISIT in some cases we could speed up erasing large regions
 	 * by using SPINOR_OP_SE instead of SPINOR_OP_BE_4K.  We may have set up
 	 * to use "small sector erase", but that's not always optimal.
@@ -315,6 +312,10 @@  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 			addr += mtd->erasesize;
 			len -= mtd->erasesize;
+
+			ret = spi_nor_wait_till_ready(nor);
+			if (ret)
+				goto erase_err;
 		}
 	}
 
@@ -342,11 +343,6 @@  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous command */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto err;
-
 	status_old = read_sr(nor);
 
 	if (offset < mtd->size - (mtd->size / 2))
@@ -389,11 +385,6 @@  static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous command */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto err;
-
 	status_old = read_sr(nor);
 
 	if (offset+len > mtd->size - (mtd->size / 64))
@@ -710,11 +701,6 @@  static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto time_out;
-
 	write_enable(nor);
 
 	nor->sst_write_second = false;
@@ -786,11 +772,6 @@  static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto write_err;
-
 	write_enable(nor);
 
 	page_offset = to & (nor->page_size - 1);
@@ -819,6 +800,7 @@  static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 		}
 	}
 
+	ret = spi_nor_wait_till_ready(nor);
 write_err:
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
 	return ret;