Skip to contentSkip to conversation

Added support for spi-nor device pm in m25p80

PATCH Superseded Kamal Dasu 3 participants 7 messages Feb 22, 2017 – Feb 23, 2017

Subsystems (3)
Memory technology devices (MTD) SPI SPI NOR

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Kamal Dasu

author
#0 lore raw

v5 cover Added support for spi-nor device pm in m25p80

Changes since v4 based on review comments:

spi-nor.c
- Added comments to make it clear that spi_nor_init() sends all spi-nor flash configuration commands based on nor stucture settings for read in the refactored code

m25p80.c:
- fix typos
- Updated description

The V4 changes below implements power management support in m25p80 driver. m25p80 pm resume() calls newly a 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 (2):
mtd: spi-nor: Added spi-nor init function
mtd: m25p80: Added pm ops support

 drivers/mtd/devices/m25p80.c  | 11 +++++++
 drivers/mtd/spi-nor/spi-nor.c | 77 ++++++++++++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   | 18 +++++++++-
 3 files changed, 78 insertions(+), 28 deletions(-)
Signature · 6 lines
1.9.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
author
#1 lore raw

PATCH v5 1/2 mtd: spi-nor: Added spi-nor init function

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

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 <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 77 ++++++++++++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   | 18 +++++++++-
 2 files changed, 67 insertions(+), 28 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +50 -27
index 70e52ff..8b71c11 100644
@@ -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);
include/linux/mtd/spi-nor.h +17 -1
index f2a7180..29a8283 100644
@@ -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{}.
  *
Signature · 6 lines
1.9.1


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

Re: [PATCH v5 1/2] mtd: spi-nor: Added spi-nor init function

Hi Kamal,

just few comments, maybe not a full review, sorry!

First, about the subject line, I know this a small detail but according to Documentation/SubmittingPatches, it should be "mtd: spi-nor: add ..."

"""
Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour.
"""

Quoted 52 lines · 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
2) set read mode to quad mode if configured such
3) set the address width based on the flash size and vendor

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 <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 77 ++++++++++++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   | 18 +++++++++-
 2 files changed, 67 insertions(+), 28 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +28 -0
index 70e52ff..8b71c11 100644
@@ -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;
+		}
+	}
+

drivers/mtd/spi-nor/spi-nor.c static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor) · line 1566

Quoted 10 lines
drivers/mtd/spi-nor/spi-nor.c +10 -0
+	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);
+

When this code was called only from spi_nor_scan(), the nor->mtd object has not been registered yet into the MTD sub-system. There was no need to lock the 'nor->lock' mutex because spi_nor_scan() can't compete with spi_nor_read(), spi_nor_write(), and so on...

However, now you plan to call this new spi_nor_init() function from the resume() handler in m25p80: the MTD device has already been registered so spi_nor_init() can now compete with spi_nor_read() and other nor->mtd.<handlers>. I guess locking the mutex might be needed now unless we have another mechanism to guarantee nor->mtd._<handlers> could never be called by the MTD sub-system when the SPI device is suspended. So, as said in my comment of patch 2 of this series, synchronization of their power states is needed between the SPI and MTD devices.

Quoted 126 lines
drivers/mtd/spi-nor/spi-nor.c +12 -27
 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);
include/linux/mtd/spi-nor.h +16 -0
index f2a7180..29a8283 100644
@@ -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);
+
+/**

include/linux/mtd/spi-nor.h static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor) · line 247

Quoted 10 lines
include/linux/mtd/spi-nor.h +1 -1
  * 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{}.
  *

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

#3 lore raw

Re: [PATCH v5 1/2] mtd: spi-nor: Added spi-nor init function

Cyrille,

Quoted 7 lines · Cyrille Pitchen

Hi Kamal,

just few comments, maybe not a full review, sorry!

First, about the subject line, I know this a small detail but according to Documentation/SubmittingPatches, it should be "mtd: spi-nor: add ..."

Quoted 7 lines

"""
Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour.
"""

Ok will make necessary changes.

Quoted 69 lines · Kamal Dasu
Quoted 52 lines

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

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 <[email protected]>
---
 drivers/mtd/spi-nor/spi-nor.c | 77 ++++++++++++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   | 18 +++++++++-
 2 files changed, 67 insertions(+), 28 deletions(-)
drivers/mtd/spi-nor/spi-nor.c +28 -0
index 70e52ff..8b71c11 100644
@@ -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;
+             }
+     }
+

drivers/mtd/spi-nor/spi-nor.c static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor) · line 1566

Quoted 10 lines
drivers/mtd/spi-nor/spi-nor.c +10 -0
+     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);
+

When this code was called only from spi_nor_scan(), the nor->mtd object has not been registered yet into the MTD sub-system. There was no need to lock the 'nor->lock' mutex because spi_nor_scan() can't compete with spi_nor_read(), spi_nor_write(), and so on...

Quoted 9 lines

However, now you plan to call this new spi_nor_init() function from the resume() handler in m25p80: the MTD device has already been registered so spi_nor_init() can now compete with spi_nor_read() and other nor->mtd.<handlers>. I guess locking the mutex might be needed now unless we have another mechanism to guarantee nor->mtd._<handlers> could never be called by the MTD sub-system when the SPI device is suspended. So, as said in my comment of patch 2 of this series, synchronization of their power states is needed between the SPI and MTD devices.

Yes I understand what you mean here. I am reworking the spi-nor driver to implement the mtd->_suspend(), mtd->_resume() hooks like other flash device driver are doing. I think it belongs here in generic spi-nor core driver. We do not need to touch the m25p80 driver.

From the spi-nor documentation the following layering is common to both the spi bus and non spi bus driver layering.

                   MTD
         ------------------------
          SPI NOR framework
         ------------------------

BTW I can see one example of fsl-quadspi driver implements resume suspend and registers its own mtd. cadence-qspi also does the same.

Quoted 129 lines
Quoted 129 lines
drivers/mtd/spi-nor/spi-nor.c +12 -27
 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);
include/linux/mtd/spi-nor.h +16 -0
index f2a7180..29a8283 100644
@@ -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
Quoted 9 lines

include/linux/mtd/spi-nor.h static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor) · line 247

Quoted 8 lines
include/linux/mtd/spi-nor.h +1 -1
  * @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{}.
  *

Thanks
Kamal

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

author
#4 lore raw

PATCH v5 2/2 mtd: m25p80: Added pm ops support

In reply to Kamal Dasu

Added power management ops for resume to be able to reinitialize spi-nor device and set it to right transfer mode in its pre-suspend state. Some SoC implementations might power down the spi-nor flash and loose its initial settings on suspend. A resume should restore part settings to its original pre-suspend state.

Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/devices/m25p80.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/mtd/devices/m25p80.c +11 -0
index c4df3b1..3ab30b2 100644
@@ -324,10 +324,21 @@ static int m25p_remove(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, m25p_of_table);
 
+#ifdef CONFIG_PM_SLEEP
+static int m25p_resume(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+
+	return spi_nor_init(&flash->spi_nor);
+}
+#endif
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, NULL, m25p_resume);
+
 static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.of_match_table = m25p_of_table,
+		.pm     = &m25p_pm_ops,
 	},
 	.id_table	= m25p_ids,
 	.probe	= m25p_probe,
Signature · 6 lines
1.9.1


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

Re: [PATCH v5 2/2] mtd: m25p80: Added pm ops support

Hi Kamal,

Quoted 28 lines · Kamal Dasu

Added power management ops for resume to be able to reinitialize spi-nor device and set it to right transfer mode in its pre-suspend state. Some SoC implementations might power down the spi-nor flash and loose its initial settings on suspend. A resume should restore part settings to its original pre-suspend state.

Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/devices/m25p80.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/mtd/devices/m25p80.c +8 -0
index c4df3b1..3ab30b2 100644
@@ -324,10 +324,21 @@ static int m25p_remove(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, m25p_of_table);
 
+#ifdef CONFIG_PM_SLEEP
+static int m25p_resume(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+
+	return spi_nor_init(&flash->spi_nor);
+}
+#endif

drivers/mtd/devices/m25p80.c static int m25p_remove(struct spi_device *spi) · line 344

Quoted 10 lines
drivers/mtd/devices/m25p80.c +3 -0
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, NULL, m25p_resume);
+
 static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.of_match_table = m25p_of_table,
+		.pm     = &m25p_pm_ops,
 	},
 	.id_table	= m25p_ids,
 	.probe	= m25p_probe,

I'm still studying the runtime_pm documentation and source code to understand how the power management works in the Linux kernel. I didn't finish but for what I have understood until now, I think there is an issue.

Here you add suspend/resume handlers on the SPI device. So the SPI sub-system is aware of the power state of the SPI flash memory, that fine. However what about the MTD sub-system? I don't see any synchronization between the SPI device and the MTD device. Hence I guess the MTD sub-system is not aware of the actual power state of the hardware memory. So I think that mtd->_read() or mtd->_write() handlers could be called from some mtd driver when the SPI device has already been suspended. For instance, let's image the root file-system is mounted from a ubifs stored a SPI NOR flash: what if the kernel tries to perform some file access when the SPI device has already been suspended?

The 'struct m25p' instance makes the link between the SPI device the the spi_nor->mtd device. Sync could be done using this object.

That why I think this patch is currently incomplete as the synchronization of the power states of both the SPI and MTD devices is missing.

I think the feature you're trying to implement is interesting but some rework seems to needed. I can't tell you more for now since, as I said, I'm still lacking strong knowledge about the runtime_pm framework.

Best regards,

Cyrille

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

#6 lore raw

Re: [PATCH v5 2/2] mtd: m25p80: Added pm ops support

Quoted 47 lines · Cyrille Pitchen

Hi Kamal,

Quoted 28 lines · Kamal Dasu

Added power management ops for resume to be able to reinitialize spi-nor device and set it to right transfer mode in its pre-suspend state. Some SoC implementations might power down the spi-nor flash and loose its initial settings on suspend. A resume should restore part settings to its original pre-suspend state.

Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/devices/m25p80.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/mtd/devices/m25p80.c +8 -0
index c4df3b1..3ab30b2 100644
@@ -324,10 +324,21 @@ static int m25p_remove(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, m25p_of_table);
 
+#ifdef CONFIG_PM_SLEEP
+static int m25p_resume(struct device *dev)
+{
+     struct m25p *flash = dev_get_drvdata(dev);
+
+     return spi_nor_init(&flash->spi_nor);
+}
+#endif

drivers/mtd/devices/m25p80.c static int m25p_remove(struct spi_device *spi) · line 344

Quoted 10 lines
drivers/mtd/devices/m25p80.c +3 -0
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, NULL, m25p_resume);
+
 static struct spi_driver m25p80_driver = {
      .driver = {
              .name   = "m25p80",
              .of_match_table = m25p_of_table,
+             .pm     = &m25p_pm_ops,
      },
      .id_table       = m25p_ids,
      .probe  = m25p_probe,

I'm still studying the runtime_pm documentation and source code to understand how the power management works in the Linux kernel. I didn't finish but for what I have understood until now, I think there is an issue.

Quoted 11 lines

Here you add suspend/resume handlers on the SPI device. So the SPI sub-system is aware of the power state of the SPI flash memory, that fine. However what about the MTD sub-system? I don't see any synchronization between the SPI device and the MTD device. Hence I guess the MTD sub-system is not aware of the actual power state of the hardware memory. So I think that mtd->_read() or mtd->_write() handlers could be called from some mtd driver when the SPI device has already been suspended. For instance, let's image the root file-system is mounted from a ubifs stored a SPI NOR flash: what if the kernel tries to perform some file access when the SPI device has already been suspended?

However in the current stack based on spi master bus driver and m25p80 flash device the spi-bcm-qspi does call the spi_master_suspend(), which stops the queue so there should not be any activity.

spi-nor may implement something on the lines of the nand_base where it simply sets a state and locks the device in a certain state using nand_get_device() call. But does not actually do any thing with the mtd structures as far as I can tell.

mtd->_suspend = spi_nor_suspend;
mtd->_resume = spi_nor_resume;
mtd->_reboot = spi_nor_shutdown;

I am not sure what spi_nor_suspend() or spi_nor_shutdown() will actually do. As mtd layer is just an abstraction in memory and does not change state. But spi-nor can be made aware of the states by maintaining it in the nor structure. Both spi and m25p80 and the controller driver are aware.

Quoted 11 lines

The 'struct m25p' instance makes the link between the SPI device the the spi_nor->mtd device. Sync could be done using this object.

That why I think this patch is currently incomplete as the synchronization of the power states of both the SPI and MTD devices is missing.

I think the feature you're trying to implement is interesting but some rework seems to needed. I can't tell you more for now since, as I said, I'm still lacking strong knowledge about the runtime_pm framework.

Ideally spi-nor driver should handle the mtd ops I thought fro the framework.

Quoted 3 lines

Best regards,

Cyrille

Thanks
Kamal

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