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

Heiner Kallweit hkallweit1 at gmail.com
Tue Nov 1 14:30:22 PDT 2016


Am 01.11.2016 um 19:58 schrieb 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?
> 
See also Documentation/timers/timers-howto.txt
msleep can be quite inaccurate when intending to sleep a few msecs only.
The threshold of 50msecs however is somewhat arbitrary, I have to admit.

In case of sleep_msecs == 0 the function behaves like before, no sleep
is inserted.

>>         }
>>
>>         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().
> 
Right, semantics of spi_nor_wait_till_ready() hasn't changed.

>>  }
>>
>>  /*
>> @@ -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?
> 
I have to admit that I don't fully understand your question. I think it's
about why different sleep time for sectore erase vs. chip erase and why
no sleep time for program operations.

Sector erase:
Typically takes 100ms+, so sleeping 2ms between each poll should be a
fair value.

Chip erase:
Takes 10s+, therefore a sleep time of 100ms should be ok.

Page program:
Takes 200us on the SPI NOR chip I deal with and I think there's not
really a benefit in inserting a sleep.

There are other places in spi-nor.c where inserting a sleep could make
sense, e.g. writing the status register takes quite a lot of time on
some chips.
However I don't have the HW to test and I didn't want to overload this
patch. If there are more use cases for the sleeping version separate
patches should be submitted.

> thanks!
> 
Thanks for review, Heiner




More information about the linux-mtd mailing list