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

[Cyrille Pitchen]

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 at 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