| Message ID | 1407374222-8448-4-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | dfa9c0cba4ea20e766bbb4f89152b05d00ab9ab3 |
| Headers | show |
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
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
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
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
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;
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(-)