Message ID | eaa526ca-f0b4-4fde-d334-2ca6af72c32d@gmail.com |
---|---|
State | Superseded |
Headers | show |
Hi Heiner, +Marek Le 30/10/2016 à 10:32, Heiner Kallweit a écrit : > Currently spi_nor_wait_till_ready in the poll loop permanently checks > for the WIP flag to be reset. Erase ops typically take >100ms for > sector erase and >10s for chip erase. Permanently polling for such > longer time periods puts unnecessary load on the SPI subsystem > (especially on systems w/o SPI DMA support or systems using bitbanging). > > Relax this by sleeping for a reasonable time between checking the > WIP flag. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index d0fc165..808ea4c 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -17,6 +17,7 @@ > #include <linux/mutex.h> > #include <linux/math64.h> > #include <linux/sizes.h> > +#include <linux/delay.h> > > #include <linux/mtd/mtd.h> > #include <linux/of_platform.h> > @@ -252,7 +253,8 @@ static int spi_nor_ready(struct spi_nor *nor) > * Returns non-zero if error. > */ > static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, > - unsigned long timeout_jiffies) > + unsigned long timeout_jiffies, > + unsigned int sleep_msecs) > { > unsigned long deadline; > int timeout = 0, ret; > @@ -269,7 +271,13 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, > if (ret) > return 0; > > - cond_resched(); > + if (!sleep_msecs) > + cond_resched(); > + else if (sleep_msecs < 50) > + usleep_range(sleep_msecs * 1000 - 100, > + sleep_msecs * 1000); > + else > + msleep(sleep_msecs); > } > > dev_err(nor->dev, "flash operation timed out\n"); > @@ -277,10 +285,17 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, > return -ETIMEDOUT; > } > > -static int spi_nor_wait_till_ready(struct spi_nor *nor) > +static inline int spi_nor_wait_till_ready_sleep(struct spi_nor *nor, > + unsigned int sleep_msecs) > { > return spi_nor_wait_till_ready_with_timeout(nor, > - DEFAULT_READY_WAIT_JIFFIES); > + DEFAULT_READY_WAIT_JIFFIES, > + sleep_msecs); > +} > + > +static inline int spi_nor_wait_till_ready(struct spi_nor *nor) > +{ > + return spi_nor_wait_till_ready_sleep(nor, 0); > } > Not a big deal but, IMHO, you should remove spi_nor_wait_till_ready_sleep() from your patch to avoid introducing a 3rd spi_nor_wait_till_ready* function, so we don't have to wonder which version to call when we have to wait for the memory to be ready again. Besides, spi_nor_wait_ready_sleep() is called only once so it is not so useful. Finally, the duration expressed by the sleep_msecs parameter should not be higher than the duration of timeout_jiffies in spi_nor_wait_till_ready_with_timeout(). The spi_nor_wait_till_ready_sleep() hides the timeout_jiffies value so it's less obvious to choose a right value for sleep_msecs when you have to look at the implementation of the spi_nor_wait_till_ready_sleep() function to find out the actual value of timeout_jiffies. What I mean is that those two parameters, timeout_jiffies and sleep_msec, are tightly bound. So when calling one of the spi_nor_wait_till_ready* functions(), those two parameters should be either both fixed (macro) or both variable. Marek, what do you think about this point? > /* > @@ -387,7 +402,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) > timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES, > CHIP_ERASE_2MB_READY_WAIT_JIFFIES * > (unsigned long)(mtd->size / SZ_2M)); > - ret = spi_nor_wait_till_ready_with_timeout(nor, timeout); > + ret = spi_nor_wait_till_ready_with_timeout(nor, timeout, 100); Please, use macros instead of hardcoded values. For instance, here it could be CHIP_ERASE_READY_WAIT_SLEEP_MSEC or something else which clearly states that the value is for chip erase operations and is associated with the CHIP_ERASE_2MB_READY_WAIT_JIFFIES macro. > if (ret) > goto erase_err; > > @@ -408,7 +423,7 @@ 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); > + ret = spi_nor_wait_till_ready_sleep(nor, 2); See comments above. Do you have some figures to show the impact of this patch on the performances? > if (ret) > goto erase_err; > } > Best regards, Cyrille
Am 31.10.2016 um 14:50 schrieb Cyrille Pitchen: > Hi Heiner, > Thanks for the very prompt review comments! > +Marek > > Le 30/10/2016 à 10:32, Heiner Kallweit a écrit : >> Currently spi_nor_wait_till_ready in the poll loop permanently checks >> for the WIP flag to be reset. Erase ops typically take >100ms for >> sector erase and >10s for chip erase. Permanently polling for such >> longer time periods puts unnecessary load on the SPI subsystem >> (especially on systems w/o SPI DMA support or systems using bitbanging). >> >> Relax this by sleeping for a reasonable time between checking the >> WIP flag. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 27 +++++++++++++++++++++------ >> 1 file changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index d0fc165..808ea4c 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -17,6 +17,7 @@ >> #include <linux/mutex.h> >> #include <linux/math64.h> >> #include <linux/sizes.h> >> +#include <linux/delay.h> >> >> #include <linux/mtd/mtd.h> >> #include <linux/of_platform.h> >> @@ -252,7 +253,8 @@ static int spi_nor_ready(struct spi_nor *nor) >> * Returns non-zero if error. >> */ >> static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, >> - unsigned long timeout_jiffies) >> + unsigned long timeout_jiffies, >> + unsigned int sleep_msecs) >> { >> unsigned long deadline; >> int timeout = 0, ret; >> @@ -269,7 +271,13 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, >> if (ret) >> return 0; >> >> - cond_resched(); >> + if (!sleep_msecs) >> + cond_resched(); >> + else if (sleep_msecs < 50) >> + usleep_range(sleep_msecs * 1000 - 100, >> + sleep_msecs * 1000); >> + else >> + msleep(sleep_msecs); >> } >> >> dev_err(nor->dev, "flash operation timed out\n"); >> @@ -277,10 +285,17 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, >> return -ETIMEDOUT; >> } >> >> -static int spi_nor_wait_till_ready(struct spi_nor *nor) >> +static inline int spi_nor_wait_till_ready_sleep(struct spi_nor *nor, >> + unsigned int sleep_msecs) >> { >> return spi_nor_wait_till_ready_with_timeout(nor, >> - DEFAULT_READY_WAIT_JIFFIES); >> + DEFAULT_READY_WAIT_JIFFIES, >> + sleep_msecs); >> +} >> + >> +static inline int spi_nor_wait_till_ready(struct spi_nor *nor) >> +{ >> + return spi_nor_wait_till_ready_sleep(nor, 0); >> } >> > > Not a big deal but, IMHO, you should remove spi_nor_wait_till_ready_sleep() > from your patch to avoid introducing a 3rd spi_nor_wait_till_ready* function, > so we don't have to wonder which version to call when we have to wait for the > memory to be ready again. > > Besides, spi_nor_wait_ready_sleep() is called only once so it is not so > useful. > > Finally, the duration expressed by the sleep_msecs parameter should not be > higher than the duration of timeout_jiffies in > spi_nor_wait_till_ready_with_timeout(). The spi_nor_wait_till_ready_sleep() > hides the timeout_jiffies value so it's less obvious to choose a right value > for sleep_msecs when you have to look at the implementation of the > spi_nor_wait_till_ready_sleep() function to find out the actual value of > timeout_jiffies. > > What I mean is that those two parameters, timeout_jiffies and sleep_msec, are > tightly bound. So when calling one of the spi_nor_wait_till_ready* functions(), > those two parameters should be either both fixed (macro) or both variable. > > Marek, what do you think about this point? > >> /* >> @@ -387,7 +402,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) >> timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES, >> CHIP_ERASE_2MB_READY_WAIT_JIFFIES * >> (unsigned long)(mtd->size / SZ_2M)); >> - ret = spi_nor_wait_till_ready_with_timeout(nor, timeout); >> + ret = spi_nor_wait_till_ready_with_timeout(nor, timeout, 100); > > Please, use macros instead of hardcoded values. For instance, here it could be > CHIP_ERASE_READY_WAIT_SLEEP_MSEC or something else which clearly states > that the value is for chip erase operations and is associated with the > CHIP_ERASE_2MB_READY_WAIT_JIFFIES macro. > >> if (ret) >> goto erase_err; >> >> @@ -408,7 +423,7 @@ 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); >> + ret = spi_nor_wait_till_ready_sleep(nor, 2); > See comments above. > > Do you have some figures to show the impact of this patch on the performances? Let's see whether Marek also has some comments, then I'll prepare a v2 of the patch. Regarding performance figures: This patch saves me ~ 3 Mio SPI interrupts (5 Mio irq -> 2 Mio irq) at first boot of OpenWRT/LEDE after flashing firmware (mainly when preparing the jffs2 file system). Boot time is the same. In detail per poll cycle (for my system setup): With 33MHz SPI clock and 2 bytes for reading the status register the SPI transfer per poll cycle takes ~ 0.5us. Plus overhead for SPI message handling etc. let's assume one poll cycle takes ~ 1us. With one interrupt per SPI transfer we then have 100.000 interrupts just for waiting for one sector erase to finish. In the patch the sleeping version is used just for sector erase and chip erase. However there are more places where using it might make sense, e.g. write_sr() takes longer time on several chips. Rgds, Heiner >> if (ret) >> goto erase_err; >> } >> > > Best regards, > > Cyrille >
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..808ea4c 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -17,6 +17,7 @@ #include <linux/mutex.h> #include <linux/math64.h> #include <linux/sizes.h> +#include <linux/delay.h> #include <linux/mtd/mtd.h> #include <linux/of_platform.h> @@ -252,7 +253,8 @@ static int spi_nor_ready(struct spi_nor *nor) * Returns non-zero if error. */ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, - unsigned long timeout_jiffies) + unsigned long timeout_jiffies, + unsigned int sleep_msecs) { unsigned long deadline; int timeout = 0, ret; @@ -269,7 +271,13 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, if (ret) return 0; - cond_resched(); + if (!sleep_msecs) + cond_resched(); + else if (sleep_msecs < 50) + usleep_range(sleep_msecs * 1000 - 100, + sleep_msecs * 1000); + else + msleep(sleep_msecs); } dev_err(nor->dev, "flash operation timed out\n"); @@ -277,10 +285,17 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, return -ETIMEDOUT; } -static int spi_nor_wait_till_ready(struct spi_nor *nor) +static inline int spi_nor_wait_till_ready_sleep(struct spi_nor *nor, + unsigned int sleep_msecs) { return spi_nor_wait_till_ready_with_timeout(nor, - DEFAULT_READY_WAIT_JIFFIES); + DEFAULT_READY_WAIT_JIFFIES, + sleep_msecs); +} + +static inline int spi_nor_wait_till_ready(struct spi_nor *nor) +{ + return spi_nor_wait_till_ready_sleep(nor, 0); } /* @@ -387,7 +402,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES, CHIP_ERASE_2MB_READY_WAIT_JIFFIES * (unsigned long)(mtd->size / SZ_2M)); - ret = spi_nor_wait_till_ready_with_timeout(nor, timeout); + ret = spi_nor_wait_till_ready_with_timeout(nor, timeout, 100); if (ret) goto erase_err; @@ -408,7 +423,7 @@ 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); + ret = spi_nor_wait_till_ready_sleep(nor, 2); if (ret) goto erase_err; }
Currently spi_nor_wait_till_ready in the poll loop permanently checks for the WIP flag to be reset. Erase ops typically take >100ms for sector erase and >10s for chip erase. Permanently polling for such longer time periods puts unnecessary load on the SPI subsystem (especially on systems w/o SPI DMA support or systems using bitbanging). Relax this by sleeping for a reasonable time between checking the WIP flag. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/mtd/spi-nor/spi-nor.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)