[PATCH v2] mtd: spi-nor: add sleeping version of spi_nor_wait_till_ready for erase ops

[Jagan Teki]

On Tue, Nov 1, 2016 at 10:35 PM, Heiner Kallweit <hkallweit1 at gmail.com> wrote:
> 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 at gmail.com>
> ---
> v2:
> - add defines for numeric constants
> - Don't introduce a third spi_nor_wait_till_ready_.. function.
> - add sanity check to ensure that sleep time <= timeout
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d0fc165..7c793a6 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>
> @@ -37,6 +38,10 @@
>   */
>  #define CHIP_ERASE_2MB_READY_WAIT_JIFFIES      (40UL * HZ)
>
> +#define READY_WAIT_NO_SLEEP                    0
> +#define SECTOR_ERASE_READY_WAIT_SLEEP_MSECS    2
> +#define CHIP_ERASE_READY_WAIT_SLEEP_MSECS      100
> +
>  #define SPI_NOR_MAX_ID_LEN     6
>  #define SPI_NOR_MAX_ADDR_WIDTH 4
>
> @@ -252,11 +257,13 @@ 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;
>
> +       sleep_msecs = min(sleep_msecs, jiffies_to_msecs(timeout_jiffies));
>         deadline = jiffies + timeout_jiffies;
>
>         while (!timeout) {
> @@ -269,7 +276,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);

Sorry, I am unable to check the different possible sleep scenarios
with these numerical checks like sleep_msecs < 50 can you explain?

>         }
>
>         dev_err(nor->dev, "flash operation timed out\n");
> @@ -280,7 +293,8 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
>  static int spi_nor_wait_till_ready(struct spi_nor *nor)
>  {
>         return spi_nor_wait_till_ready_with_timeout(nor,
> -                                                   DEFAULT_READY_WAIT_JIFFIES);
> +                                                   DEFAULT_READY_WAIT_JIFFIES,
> +                                                   READY_WAIT_NO_SLEEP);

So this should identical as with previous behaviour of no-sleep isn't
it? where it eventually call the cond_resched().

>  }
>
>  /*
> @@ -387,7 +401,8 @@ 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,
> +                                       CHIP_ERASE_READY_WAIT_SLEEP_MSECS);
>                 if (ret)
>                         goto erase_err;
>
> @@ -408,7 +423,9 @@ 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_with_timeout(nor,
> +                                       DEFAULT_READY_WAIT_JIFFIES,
> +                                       SECTOR_ERASE_READY_WAIT_SLEEP_MSECS);

I don't think sleeping version should require for sector-erase unlike
the chip-erase sector-erase enough of using default ready_wait jiffies
which eventually similar ready_wait for program operations as well.
did you find any diff while testing this?

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.