linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/3] Add spi-nor flash device pm support
@ 2017-02-24 20:16 Kamal Dasu
  2017-02-24 20:16 ` [PATCH v6 1/3] mtd: spi-nor: Add spi-nor init function Kamal Dasu
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Kamal Dasu @ 2017-02-24 20:16 UTC (permalink / raw)
  To: linux-spi, cyrille.pitchen, marex, broonie
  Cc: bcm-kernel-feedback-list, f.fainelli, linux-mtd, Kamal Dasu

Changes since v5 based on review comments:

spi-nor.h
- Added flash state field
- Added spin_lock for flash state change 
- Added wait queue for synchronization of spi-nor ops between 
  mtd and spi-nor driver

spi-nor.c
- Added spi_nor_get/release_device() flash state syncronization functions
- Added mtd suspend and resume handlers

rerverted m25p80.c changes 

The V5 changes below implements power management support using the mtd
handlers for resume and suspend in the spi-nor driver. Implemented flash
state synchronization methods based on other mtd device drivers.
spi-nor mtd pm resume() calls newly implemented spi_nor_init() function
that sets up the spi-nor flash to its pre-suspend state. This is needed
on platfroms that turn off power to the spi-nor flash on pm suspend. 
The code has been rebased to git://github.com/spi-nor/linux.git next
branch.

Kamal Dasu (3):
  mtd: spi-nor: Add spi-nor init function
  mtd: spi-nor: Add spi-nor flash device synchronization between flash
    states
  mtd: spi-nor: Add spi-nor mtd suspend and resume handlers

 drivers/mtd/spi-nor/spi-nor.c | 170 +++++++++++++++++++++++++++++++++++-------
 include/linux/mtd/spi-nor.h   |  22 +++++-
 2 files changed, 164 insertions(+), 28 deletions(-)

-- 
1.9.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v6 1/3] mtd: spi-nor: Add spi-nor init function
  2017-02-24 20:16 [PATCH v6 0/3] Add spi-nor flash device pm support Kamal Dasu
@ 2017-02-24 20:16 ` Kamal Dasu
  2017-02-24 20:16 ` [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states Kamal Dasu
       [not found] ` <1487967399-28967-1-git-send-email-kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2 siblings, 0 replies; 9+ messages in thread
From: Kamal Dasu @ 2017-02-24 20:16 UTC (permalink / raw)
  To: linux-spi, cyrille.pitchen, marex, broonie
  Cc: bcm-kernel-feedback-list, f.fainelli, linux-mtd, Kamal Dasu

Refactored spi_nor_scan() code to add spi_nor_init() function that
programs the spi-nor flash to:
1) remove flash protection if applicable for certain vendors
2) set read mode to quad mode if configured such
3) set the address width based on the flash size

spi_nor_scan() now calls spi_nor_init() to setup nor flash.
The init function can also be called by flash device driver
to reinitialize spi-nor flash.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 77 ++++++++++++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   | 18 +++++++++-
 2 files changed, 67 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 70e52ff..8b71c11 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1526,6 +1526,44 @@ static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
 	return 0;
 }
 
+int spi_nor_init(struct spi_nor *nor)
+{
+	int ret = 0;
+	const struct flash_info *info = nor->info;
+	struct device *dev = nor->dev;
+
+	/*
+	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
+	 * with the software protection bits set
+	 */
+
+	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
+	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
+	    JEDEC_MFR(info) == SNOR_MFR_SST ||
+	    info->flags & SPI_NOR_HAS_LOCK) {
+		write_enable(nor);
+		write_sr(nor, 0);
+		spi_nor_wait_till_ready(nor);
+	}
+
+	if (nor->flash_read == SPI_NOR_QUAD) {
+		ret = set_quad_mode(nor, info);
+		if (ret) {
+			dev_err(dev, "quad mode not supported\n");
+			return ret;
+		}
+	}
+
+	if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
+	    info->flags & SPI_NOR_4B_OPCODES)
+		spi_nor_set_4byte_opcodes(nor, info);
+	else
+		set_4byte(nor, info, 1);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_nor_init);
+
 int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 {
 	const struct flash_info *info = NULL;
@@ -1571,6 +1609,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 		}
 	}
 
+	/* update pointer to flash_info in the nor structure */
+	nor->info = info;
 	mutex_init(&nor->lock);
 
 	/*
@@ -1581,20 +1621,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	if (info->flags & SPI_S3AN)
 		nor->flags |=  SNOR_F_READY_XSR_RDY;
 
-	/*
-	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
-	 * with the software protection bits set
-	 */
-
-	if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
-	    JEDEC_MFR(info) == SNOR_MFR_INTEL ||
-	    JEDEC_MFR(info) == SNOR_MFR_SST ||
-	    info->flags & SPI_NOR_HAS_LOCK) {
-		write_enable(nor);
-		write_sr(nor, 0);
-		spi_nor_wait_till_ready(nor);
-	}
-
 	if (!mtd->name)
 		mtd->name = dev_name(dev);
 	mtd->priv = nor;
@@ -1669,11 +1695,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	/* Quad/Dual-read mode takes precedence over fast/normal */
 	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
-		ret = set_quad_mode(nor, info);
-		if (ret) {
-			dev_err(dev, "quad mode not supported\n");
-			return ret;
-		}
 		nor->flash_read = SPI_NOR_QUAD;
 	} else if (mode == SPI_NOR_DUAL && info->flags & SPI_NOR_DUAL_READ) {
 		nor->flash_read = SPI_NOR_DUAL;
@@ -1702,17 +1723,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	if (info->addr_width)
 		nor->addr_width = info->addr_width;
-	else if (mtd->size > 0x1000000) {
+	else if (mtd->size > 0x1000000)
 		/* enable 4-byte addressing if the device exceeds 16MiB */
 		nor->addr_width = 4;
-		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
-		    info->flags & SPI_NOR_4B_OPCODES)
-			spi_nor_set_4byte_opcodes(nor, info);
-		else
-			set_4byte(nor, info, 1);
-	} else {
+	else
 		nor->addr_width = 3;
-	}
 
 	if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) {
 		dev_err(dev, "address width is too large: %u\n",
@@ -1728,6 +1743,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 			return ret;
 	}
 
+	/*
+	 * call init function to send necessary spi-nor read/write config
+	 * commands to nor flash based on above nor settings
+	 */
+	ret = spi_nor_init(nor);
+	if (ret)
+		return ret;
+
 	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
 			(long long)mtd->size >> 10);
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index f2a7180..29a8283 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -143,6 +143,8 @@ enum spi_nor_option_flags {
 	SNOR_F_READY_XSR_RDY	= BIT(4),
 };
 
+struct flash_info;
+
 /**
  * struct spi_nor - Structure for defining a the SPI NOR layer
  * @mtd:		point to a mtd_info structure
@@ -174,6 +176,7 @@ enum spi_nor_option_flags {
  * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
  *			completely locked
  * @priv:		the private data
+ * @info:		points to the flash_info structure
  */
 struct spi_nor {
 	struct mtd_info		mtd;
@@ -206,6 +209,7 @@ struct spi_nor {
 	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 
 	void *priv;
+	const struct flash_info *info;
 };
 
 static inline void spi_nor_set_flash_node(struct spi_nor *nor,
@@ -220,12 +224,24 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
 }
 
 /**
+ * spi_nor_init() - initialize SPI NOR
+ * @nor:	the spi_nor structure
+ *
+ * The drivers uses this function to initialize the SPI NOR flash device to
+ * settings in spi_nor structure. The functions sets read mode, address width
+ * and removes protection on the flash device based on those settings.
+ *
+ * Return: 0 for success, others for failure.
+ */
+int spi_nor_init(struct spi_nor *nor);
+
+/**
  * spi_nor_scan() - scan the SPI NOR
  * @nor:	the spi_nor structure
  * @name:	the chip type name
  * @mode:	the read mode supported by the driver
  *
- * The drivers can use this fuction to scan the SPI NOR.
+ * The drivers can use this function to scan the SPI NOR.
  * In the scanning, it will try to get all the necessary information to
  * fill the mtd_info{} and the spi_nor{}.
  *
-- 
1.9.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states
  2017-02-24 20:16 [PATCH v6 0/3] Add spi-nor flash device pm support Kamal Dasu
  2017-02-24 20:16 ` [PATCH v6 1/3] mtd: spi-nor: Add spi-nor init function Kamal Dasu
@ 2017-02-24 20:16 ` Kamal Dasu
  2017-02-26 12:18   ` Marek Vasut
  2017-03-07 23:08   ` Cyrille Pitchen
       [not found] ` <1487967399-28967-1-git-send-email-kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2 siblings, 2 replies; 9+ messages in thread
From: Kamal Dasu @ 2017-02-24 20:16 UTC (permalink / raw)
  To: linux-spi, cyrille.pitchen, marex, broonie
  Cc: bcm-kernel-feedback-list, f.fainelli, linux-mtd, Kamal Dasu

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.

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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v6 3/3] mtd: spi-nor: Add spi-nor mtd suspend and resume handlers
       [not found] ` <1487967399-28967-1-git-send-email-kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-02-24 20:16   ` Kamal Dasu
  0 siblings, 0 replies; 9+ messages in thread
From: Kamal Dasu @ 2017-02-24 20:16 UTC (permalink / raw)
  To: linux-spi-u79uwXL29TY76Z2rM5mHXA,
	cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w, marex-ynQEQJNshbs,
	broonie-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Kamal Dasu

Implemented and populated spi-nor mtd PM handlers for suspend and resume ops.
spi-nor suspend op set's the flash state to FL_PM_SUSPENDED state and resume
op re-initializes spi-nor flash and set's flash state to FL_READY. The handlers
ensures synchronization between the spi-nor and the mtd layer by calling
spi_nor_get/release_device() and setting respective flash states.

Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/spi-nor/spi-nor.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 5363807..8e38895 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1629,6 +1629,28 @@ int spi_nor_init(struct spi_nor *nor)
 }
 EXPORT_SYMBOL_GPL(spi_nor_init);
 
+/* mtd suspend handler */
+static int spi_nor_suspend(struct mtd_info *mtd)
+{
+	return spi_nor_get_device(mtd, FL_PM_SUSPENDED);
+}
+
+/* mtd resume handler */
+static void spi_nor_resume(struct mtd_info *mtd)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	struct device *dev = nor->dev;
+
+	if (nor->state == FL_PM_SUSPENDED) {
+		/* re-initialize the nor chip */
+		spi_nor_init(nor);
+		/* mtd can resume transfers */
+		spi_nor_release_device(mtd);
+	} else {
+	       dev_err(dev, "resume() called, chip not in suspended state\n");
+	}
+}
+
 int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 {
 	const struct flash_info *info = NULL;
@@ -1695,6 +1717,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mtd->size = info->sector_size * info->n_sectors;
 	mtd->_erase = spi_nor_erase;
 	mtd->_read = spi_nor_read;
+	mtd->_suspend = spi_nor_suspend;
+	mtd->_resume = spi_nor_resume;
 
 	/* NOR protection support for STmicro/Micron chips and similar */
 	if (JEDEC_MFR(info) == SNOR_MFR_MICRON ||
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states
  2017-02-24 20:16 ` [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states Kamal Dasu
@ 2017-02-26 12:18   ` Marek Vasut
       [not found]     ` <7500eaf2-082d-2902-b6b5-768189d1aae9-ynQEQJNshbs@public.gmane.org>
  2017-03-07 23:08   ` Cyrille Pitchen
  1 sibling, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2017-02-26 12:18 UTC (permalink / raw)
  To: Kamal Dasu, linux-spi, cyrille.pitchen, broonie
  Cc: bcm-kernel-feedback-list, f.fainelli, linux-mtd

On 02/24/2017 09:16 PM, Kamal Dasu wrote:
> Added flash access synchronization methods spi_nor_get/release_device(). This

Should be get/put to be consistent with the rest of the kernel ...

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

Can't you just retain which instruction is in progress instead of adding
yet another orthogonal FL_FOO ?

>  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);

Somehow, I have to wonder, doesn't runtime PM implement this sort of
functionality already ?

> +	}
> +
> +	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,
> 


-- 
Best regards,
Marek Vasut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states
       [not found]     ` <7500eaf2-082d-2902-b6b5-768189d1aae9-ynQEQJNshbs@public.gmane.org>
@ 2017-03-03 21:38       ` Kamal Dasu
       [not found]         ` <CAC=U0a04jNhP1uWB6jMvDE-D7j7cgvdSG1DazQiU+mJmTwC7eQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Kamal Dasu @ 2017-03-03 21:38 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-spi, Cyrille Pitchen, Mark Brown, MTD Maling List,
	Florian Fainelli,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w

Marek,

On Sun, Feb 26, 2017 at 7:18 AM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> On 02/24/2017 09:16 PM, Kamal Dasu wrote:
>> Added flash access synchronization methods spi_nor_get/release_device(). This
>
> Should be get/put to be consistent with the rest of the kernel ...
>

Can change that.

>> 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.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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,
>> +};
>
> Can't you just retain which instruction is in progress instead of adding
> yet another orthogonal FL_FOO ?
>

I just used what other mtd flash drivers use. I could use the ops, but
will have to add the missing ops/states.

>>  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);
>
> Somehow, I have to wonder, doesn't runtime PM implement this sort of
> functionality already ?
>

I did not see any API I could apply here.

>> +     }
>> +
>> +     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,
>>
>
>
> --
> Best regards,
> Marek Vasut

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states
       [not found]         ` <CAC=U0a04jNhP1uWB6jMvDE-D7j7cgvdSG1DazQiU+mJmTwC7eQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-03-05  1:11           ` Marek Vasut
  0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2017-03-05  1:11 UTC (permalink / raw)
  To: Kamal Dasu
  Cc: linux-spi, Cyrille Pitchen, Mark Brown, MTD Maling List,
	Florian Fainelli,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w

On 03/03/2017 10:38 PM, Kamal Dasu wrote:
> Marek,
>
> On Sun, Feb 26, 2017 at 7:18 AM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> On 02/24/2017 09:16 PM, Kamal Dasu wrote:
>>> Added flash access synchronization methods spi_nor_get/release_device(). This
>>
>> Should be get/put to be consistent with the rest of the kernel ...
>>
>
> Can change that.
>
>>> 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.
>>>
>>> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  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,
>>> +};
>>
>> Can't you just retain which instruction is in progress instead of adding
>> yet another orthogonal FL_FOO ?
>>
>
> I just used what other mtd flash drivers use. I could use the ops, but
> will have to add the missing ops/states.

What is missing ?

>>>  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);
>>
>> Somehow, I have to wonder, doesn't runtime PM implement this sort of
>> functionality already ?
>>
>
> I did not see any API I could apply here.

Does runtime_pm_get()/runtime_pm_put() help here ?

[...]

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states
  2017-02-24 20:16 ` [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states Kamal Dasu
  2017-02-26 12:18   ` Marek Vasut
@ 2017-03-07 23:08   ` Cyrille Pitchen
       [not found]     ` <79e81e11-3073-7078-05ca-3201f6dac15f-yU5RGvR974pGWvitb5QawA@public.gmane.org>
  1 sibling, 1 reply; 9+ messages in thread
From: Cyrille Pitchen @ 2017-03-07 23:08 UTC (permalink / raw)
  To: Kamal Dasu, linux-spi, cyrille.pitchen, marex, broonie, Boris Brezillon
  Cc: linux-mtd, f.fainelli, bcm-kernel-feedback-list

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@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,
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states
       [not found]     ` <79e81e11-3073-7078-05ca-3201f6dac15f-yU5RGvR974pGWvitb5QawA@public.gmane.org>
@ 2017-03-09 20:03       ` Kamal Dasu
  0 siblings, 0 replies; 9+ messages in thread
From: Kamal Dasu @ 2017-03-09 20:03 UTC (permalink / raw)
  To: Cyrille Pitchen
  Cc: Kamal Dasu, linux-spi-u79uwXL29TY76Z2rM5mHXA, Cyrille Pitchen,
	Marek Vasut, Mark Brown, Boris Brezillon,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
	Florian Fainelli, MTD Maling List

On Tue, Mar 7, 2017 at 6:08 PM, Cyrille Pitchen
<cyrille.pitchen-yU5RGvR974pGWvitb5QawA@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-03-09 20:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-24 20:16 [PATCH v6 0/3] Add spi-nor flash device pm support Kamal Dasu
2017-02-24 20:16 ` [PATCH v6 1/3] mtd: spi-nor: Add spi-nor init function Kamal Dasu
2017-02-24 20:16 ` [PATCH v6 2/3] mtd: spi-nor: Add spi-nor flash device synchronization between flash states Kamal Dasu
2017-02-26 12:18   ` Marek Vasut
     [not found]     ` <7500eaf2-082d-2902-b6b5-768189d1aae9-ynQEQJNshbs@public.gmane.org>
2017-03-03 21:38       ` Kamal Dasu
     [not found]         ` <CAC=U0a04jNhP1uWB6jMvDE-D7j7cgvdSG1DazQiU+mJmTwC7eQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-03-05  1:11           ` Marek Vasut
2017-03-07 23:08   ` Cyrille Pitchen
     [not found]     ` <79e81e11-3073-7078-05ca-3201f6dac15f-yU5RGvR974pGWvitb5QawA@public.gmane.org>
2017-03-09 20:03       ` Kamal Dasu
     [not found] ` <1487967399-28967-1-git-send-email-kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-02-24 20:16   ` [PATCH v6 3/3] mtd: spi-nor: Add spi-nor mtd suspend and resume handlers Kamal Dasu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).