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

[Cyrille Pitchen <cyrille.pitchen@xxxxxxxxxx>]

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.

Best regards,

Cyrille


> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
> ---
>  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,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html