[PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states

Kamal Dasu kamal.dasu at broadcom.com
Thu Mar 9 12:03:09 PST 2017


On Tue, Mar 7, 2017 at 6:08 PM, Cyrille Pitchen
<cyrille.pitchen at wedev4u.fr> wrote:
> Hi Kamal,
>
> +Boris for his knowledge on power management.
>
> Le 24/02/2017 à 21:16, Kamal Dasu a écrit :
>> Added flash access synchronization methods spi_nor_get/release_device(). This
>> change allows spi-nor driver to maintain flash states in spi-nor stucture for
>> read, write, erase, lock, unlock nor ops. Only when the flash state is FL_READY
>> a new state is set and spi-nor flash op is initiated. The state change is done
>> with a spin_lock held and released as soon as the state is changed. Else the
>> current process for spi-nor transfer is queued till the flash is in FL_READY
>> state. This change allows us to add mtd layer synchronization when the mtd
>> resume suspend handlers are added.
>>
>
> This version goes the wrong way, IMHO. This is not what I meant when I
> was talking about synchronizing MTD and SPI devices: when accessing the
> MTD device we should wake the SPI device up instead of blocking the MTD
> handlers when the SPI device enters its suspended mode.
>
> However, the good news is that looking closer at how the runtime pm
> works, this synchronization should already be managed:
>
> in drivers/mtd/devices/m25p80.c:
>
>         nor->dev = &spi->dev;
>
> and in drivers/mtd/spi-nor/spi-nor.c:
>
>         struct device *dev = nor->dev;
>         [...]
>         mtd->dev.parent = dev;
>
> Hence the SPI device is the parent of the MTD device.
> Then by default, when resuming a device, the runtime pm framework wakes
> the device parent up too: see rmp_resume() from drivers/base/power/runtime.c
>
> About the SPI controller, by default SPI masters ignore their children,
> so the SPI controller won't be resumed by the m25p80 SPI device. However
> spi_sync() can resume the controller when master->auto_runtime_pm is true.
>
> Then back to the MTD subsystem, maybe you should patch the mtdcore.c
> file rather than m25p80.c so the whole MTD subsystem could take
> advantage of your work on power management:
> you could update the .pm member of 'struct class' mtd_class to add
> runtime pm support to the already existing system-wide power management
> support. I guess you will have to add new hooks such mtd->_pm_suspend
> and mtd->_pm_resume. Finally spi-nor.c will implement those 2 hooks (and
> the NAND subsystem may also implement them as needed).
>
> Boris, does it make sense?
>
> Also, one last comment: I guess a call to pm_runtime_enable() is missing
> somewhere is this series, otherwise I don't think this could work.
> Besides, depending on where it will be added, we should not call
> pm_runtime_enable() unconditionally thinking of users who don't want to
> use this feature sending unneeded SPI commands to the memory.
>

To be clear I was implementing  for system sleep and S2/S3 pm modes,
so mtd_resume() and mtd_suspend() or the m25p80 suspend/resume is what
I need. I am not not having a run_time_pm  requirement.


> Best regards,
>
> Cyrille
>

Kamal
>
>> Signed-off-by: Kamal Dasu <kdasu.kdev at gmail.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 69 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h   |  4 +++
>>  2 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 8b71c11..5363807 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -89,6 +89,15 @@ struct flash_info {
>>
>>  #define JEDEC_MFR(info)      ((info)->id[0])
>>
>> +/* map table for spi_nor op to flashchip state  */
>> +static int spi_nor_state[] = {
>> +     [SPI_NOR_OPS_READ]      = FL_READING,
>> +     [SPI_NOR_OPS_WRITE]     = FL_WRITING,
>> +     [SPI_NOR_OPS_ERASE]     = FL_ERASING,
>> +     [SPI_NOR_OPS_LOCK]      = FL_LOCKING,
>> +     [SPI_NOR_OPS_UNLOCK]    = FL_UNLOCKING,
>> +};
>> +
>>  static const struct flash_info *spi_nor_match_id(const char *name);
>>
>>  /*
>> @@ -396,10 +405,64 @@ static int erase_chip(struct spi_nor *nor)
>>       return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0);
>>  }
>>
>> +/**
>> + * spi_nor_get_device - [GENERIC] Get chip for selected access
>> + * @param mtd                MTD device structure
>> + * @param new_state  the state which is requested
>> + *
>> + * Get the nor flash device and lock it for exclusive access
>> + */
>> +static int spi_nor_get_device(struct mtd_info *mtd, int new_state)
>> +{
>> +     struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +     DECLARE_WAITQUEUE(wait, current);
>> +
>> +     /*
>> +      * Grab the lock and see if the device is available
>> +      */
>> +     while (1) {
>> +             spin_lock(&nor->chip_lock);
>> +             if (nor->state == FL_READY) {
>> +                     nor->state = new_state;
>> +                     spin_unlock(&nor->chip_lock);
>> +                     break;
>> +             }
>> +             if (new_state == FL_PM_SUSPENDED) {
>> +                     spin_unlock(&nor->chip_lock);
>> +                     return (nor->state == FL_PM_SUSPENDED) ? 0 : -EAGAIN;
>> +             }
>> +             set_current_state(TASK_UNINTERRUPTIBLE);
>> +             add_wait_queue(&nor->wq, &wait);
>> +             spin_unlock(&nor->chip_lock);
>> +             schedule();
>> +             remove_wait_queue(&nor->wq, &wait);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * spi_nor_release_device - [GENERIC] release chip
>> + * @mtd: MTD device structure
>> + *
>> + * Release nor flash chip lock and wake up anyone waiting on the device.
>> + */
>> +static void spi_nor_release_device(struct mtd_info *mtd)
>> +{
>> +     struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +
>> +     /* Release the controller and the chip */
>> +     spin_lock(&nor->chip_lock);
>> +     nor->state = FL_READY;
>> +     wake_up(&nor->wq);
>> +     spin_unlock(&nor->chip_lock);
>> +}
>> +
>>  static int spi_nor_lock_and_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>  {
>>       int ret = 0;
>>
>> +     spi_nor_get_device(&nor->mtd, spi_nor_state[ops]);
>>       mutex_lock(&nor->lock);
>>
>>       if (nor->prepare) {
>> @@ -407,6 +470,7 @@ static int spi_nor_lock_and_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>               if (ret) {
>>                       dev_err(nor->dev, "failed in the preparation.\n");
>>                       mutex_unlock(&nor->lock);
>> +                     spi_nor_release_device(&nor->mtd);
>>                       return ret;
>>               }
>>       }
>> @@ -418,6 +482,7 @@ static void spi_nor_unlock_and_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>       if (nor->unprepare)
>>               nor->unprepare(nor, ops);
>>       mutex_unlock(&nor->lock);
>> +     spi_nor_release_device(&nor->mtd);
>>  }
>>
>>  /*
>> @@ -1743,6 +1808,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>                       return ret;
>>       }
>>
>> +     nor->state = FL_READY;
>> +     init_waitqueue_head(&nor->wq);
>> +     spin_lock_init(&nor->chip_lock);
>> +
>>       /*
>>        * call init function to send necessary spi-nor read/write config
>>        * commands to nor flash based on above nor settings
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 29a8283..244d98d 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -13,6 +13,7 @@
>>  #include <linux/bitops.h>
>>  #include <linux/mtd/cfi.h>
>>  #include <linux/mtd/mtd.h>
>> +#include <linux/mtd/flashchip.h>
>>
>>  /*
>>   * Manufacturer IDs
>> @@ -210,6 +211,9 @@ struct spi_nor {
>>
>>       void *priv;
>>       const struct flash_info *info;
>> +     spinlock_t              chip_lock;
>> +     wait_queue_head_t       wq;
>> +     flstate_t               state;
>>  };
>>
>>  static inline void spi_nor_set_flash_node(struct spi_nor *nor,
>>
>



More information about the linux-mtd mailing list