[PATCH v2 0/2] Added support for spi-nor device pm in m25p80

[PATCH v2 0/2] Added support for spi-nor device pm in m25p80

[Kamal Dasu]

The changes below implements power management support in m25p80 driver.
m25p80 pm resume() calls newly a implemented spi_nor_reset() function
that sets up the spi-nor flash to its probed state. This is needed on
platfroms that turn off power to the spi-nor flash on pm suspend. 

Kamal Dasu (2):
  mtd: spi-nor: Added spi-nor reset function
  mtd: m25p80: Added pm ops support

 drivers/mtd/devices/m25p80.c  | 16 +++++++++++
 drivers/mtd/spi-nor/spi-nor.c | 62 ++++++++++++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   | 21 ++++++++++++++-
 3 files changed, 77 insertions(+), 22 deletions(-)

-- 
1.9.1


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

[PATCH v2 1/2] mtd: spi-nor: Added spi-nor reset function

[Kamal Dasu]

Refactored spi_nor_scan() code to add spi_nor_reset() function that
programs the nand device 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

On pm resume spi-nor flash may need to be reconfigured after power
reset, there is no need to go through a full spi_nor_scan(), flash
device driver needs to call spi_nor_reset() to reprogram the flash
to its probed state.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index da7cd69..8e3d8bd 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1312,6 +1312,41 @@ static int spi_nor_check(struct spi_nor *nor)
 	return 0;
 }
 
+int spi_nor_reset(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 (nor->addr_width == 4 && JEDEC_MFR(info) != SNOR_MFR_SPANSION)
+		set_4byte(nor, info, 1);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_nor_reset);
+
 int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 {
 	const struct flash_info *info = NULL;
@@ -1357,22 +1392,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 		}
 	}
 
+	nor->info = info;
 	mutex_init(&nor->lock);
 
-	/*
-	 * 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;
@@ -1447,11 +1469,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;
@@ -1503,8 +1520,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 			/* No small sector erase for 4-byte command set */
 			nor->erase_opcode = SPINOR_OP_SE_4B;
 			mtd->erasesize = info->sector_size;
-		} else
-			set_4byte(nor, info, 1);
+		}
 	} else {
 		nor->addr_width = 3;
 	}
@@ -1517,6 +1533,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	nor->read_dummy = spi_nor_read_dummy_cycles(nor);
 
+	ret = spi_nor_reset(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 c425c7b..4733c04 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -121,6 +121,8 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_SR_TB	= BIT(1),
 };
 
+struct flash_info;
+
 /**
  * struct spi_nor - Structure for defining a the SPI NOR layer
  * @mtd:		point to a mtd_info structure
@@ -154,6 +156,7 @@ enum spi_nor_option_flags {
  * @priv:		the private data
  */
 struct spi_nor {
+	const struct flash_info *info;
 	struct mtd_info		mtd;
 	struct mutex		lock;
 	struct device		*dev;
@@ -198,12 +201,28 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
 }
 
 /**
+ * spi_nor_reset() - reset scan the SPI NOR
+ * @nor:	the spi_nor structure
+ *
+ * The drivers uses this function to reset the SPI NOR flash device to
+ * its initial scanned state, it shall use all nor information set on poweron
+ * for the read mode, address width and enabling write mode for certain
+ * manufacturers. This would be needed to be called for flash devices that are
+ * reset during power management.
+ *
+ * The chip type name can be provided through the @name parameter.
+ *
+ * Return: 0 for success, others for failure.
+ */
+int spi_nor_reset(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/

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

[Kamal Dasu]

Added power management ops for resume to be able to resan spi-nor
device and set it to right transfer modes in its probed state after
poweron. Some SoC implementations might  power down the spi-nor flash
and loose its initial settings on suspend. A resume should retore the
part to its probed state.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd..69d309b 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -319,10 +319,26 @@ static int m25p_remove(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, m25p_of_table);
 
+#ifdef CONFIG_PM_SLEEP
+static int m25p_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int m25p_resume(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+
+	return spi_nor_reset(&flash->spi_nor);
+}
+#endif
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, 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,
-- 
1.9.1


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

Re: [PATCH v2 1/2] mtd: spi-nor: Added spi-nor reset function

[Marek Vasut]

On 02/08/2017 05:53 PM, Kamal Dasu wrote:
> Refactored spi_nor_scan() code to add spi_nor_reset() function that
> programs the nand device to:

nand device ?

> 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
> 
> On pm resume spi-nor flash may need to be reconfigured after power
> reset, there is no need to go through a full spi_nor_scan(), flash
> device driver needs to call spi_nor_reset() to reprogram the flash
> to its probed state.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 62 ++++++++++++++++++++++++++++---------------
>  include/linux/mtd/spi-nor.h   | 21 ++++++++++++++-
>  2 files changed, 61 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index da7cd69..8e3d8bd 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1312,6 +1312,41 @@ static int spi_nor_check(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +int spi_nor_reset(struct spi_nor *nor)
> +{
> +	int ret = 0;
> +	const struct flash_info *info = nor->info;
> +	struct device *dev = nor->dev;

Does this reset the SPI NOR (as in, really send a reset opcode) ? It
doesn't, so the naming is confusing. This should probably be something
like "spi_nor_init()" or "spi_nor_setup()" , no ?

> +	/*
> +	 * 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 (nor->addr_width == 4 && JEDEC_MFR(info) != SNOR_MFR_SPANSION)
> +		set_4byte(nor, info, 1);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(spi_nor_reset);
> +
>  int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  {
>  	const struct flash_info *info = NULL;
> @@ -1357,22 +1392,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  		}
>  	}
>  
> +	nor->info = info;
>  	mutex_init(&nor->lock);
>  
> -	/*
> -	 * 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;
> @@ -1447,11 +1469,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;
> @@ -1503,8 +1520,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  			/* No small sector erase for 4-byte command set */
>  			nor->erase_opcode = SPINOR_OP_SE_4B;
>  			mtd->erasesize = info->sector_size;
> -		} else
> -			set_4byte(nor, info, 1);
> +		}
>  	} else {
>  		nor->addr_width = 3;
>  	}
> @@ -1517,6 +1533,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  
>  	nor->read_dummy = spi_nor_read_dummy_cycles(nor);
>  
> +	ret = spi_nor_reset(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 c425c7b..4733c04 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -121,6 +121,8 @@ enum spi_nor_option_flags {
>  	SNOR_F_HAS_SR_TB	= BIT(1),
>  };
>  
> +struct flash_info;
> +
>  /**
>   * struct spi_nor - Structure for defining a the SPI NOR layer
>   * @mtd:		point to a mtd_info structure
> @@ -154,6 +156,7 @@ enum spi_nor_option_flags {
>   * @priv:		the private data
>   */
>  struct spi_nor {
> +	const struct flash_info *info;
>  	struct mtd_info		mtd;
>  	struct mutex		lock;
>  	struct device		*dev;
> @@ -198,12 +201,28 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
>  }
>  
>  /**
> + * spi_nor_reset() - reset scan the SPI NOR
> + * @nor:	the spi_nor structure
> + *
> + * The drivers uses this function to reset the SPI NOR flash device to
> + * its initial scanned state, it shall use all nor information set on poweron
> + * for the read mode, address width and enabling write mode for certain
> + * manufacturers. This would be needed to be called for flash devices that are
> + * reset during power management.
> + *
> + * The chip type name can be provided through the @name parameter.
> + *
> + * Return: 0 for success, others for failure.
> + */
> +int spi_nor_reset(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{}.
>   *
> 


-- 
Best regards,
Marek Vasut

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

Re: [PATCH v2 1/2] mtd: spi-nor: Added spi-nor reset function

[Cyrille Pitchen]

Hi Kamal,

Le 08/02/2017 à 17:53, Kamal Dasu a écrit :
> Refactored spi_nor_scan() code to add spi_nor_reset() function that
> programs the nand device 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
> 
> On pm resume spi-nor flash may need to be reconfigured after power
> reset, there is no need to go through a full spi_nor_scan(), flash
> device driver needs to call spi_nor_reset() to reprogram the flash
> to its probed state.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 62 ++++++++++++++++++++++++++++---------------
>  include/linux/mtd/spi-nor.h   | 21 ++++++++++++++-
>  2 files changed, 61 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index da7cd69..8e3d8bd 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1312,6 +1312,41 @@ static int spi_nor_check(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +int spi_nor_reset(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 (nor->addr_width == 4 && JEDEC_MFR(info) != SNOR_MFR_SPANSION)
> +		set_4byte(nor, info, 1);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(spi_nor_reset);
> +
>  int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  {
>  	const struct flash_info *info = NULL;
> @@ -1357,22 +1392,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  		}
>  	}
>  
> +	nor->info = info;
>  	mutex_init(&nor->lock);
>  
> -	/*
> -	 * 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;
> @@ -1447,11 +1469,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;
> @@ -1503,8 +1520,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  			/* No small sector erase for 4-byte command set */
>  			nor->erase_opcode = SPINOR_OP_SE_4B;
>  			mtd->erasesize = info->sector_size;

It seems that you didn't base your patch on the spi-nor tree. Before your
patch, you should have something like:

	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 {
		nor->addr_width = 3;
	}

That's why in my comment of review 1, I was talking about handling the
case where spi_nor_set_4byte_opcodes() is called instead of set_4byte():
indeed when the manufacturer id is Spansion or when the SPI_NOR_4B_OPCODES
flag is set.

Hence in spi_nor_reset(), or whatever name you choose, set_4byte() must not
be called if the SPI_NOR_4B_OPCODES flag is set in info->flags.

Besides, for now I don't know that much about how power management is
handled by the Linux kernel, especially which events trigger the call of
.suspend() or .resume().
I've started to read documentation about that topic so please be patient
because I need some time to figure out the potential side effects this
series may introduce.
I know you test your patches before submitting them, so this is not a
question of trust but I'd rather understand what patches actually do
instead of blindly merge them! :)

Also maybe this patch can help you:
http://patchwork.ozlabs.org/patch/719777/

It's not related to power management but it also reworks spi_nor_scan()
so all settings (op codes, protocols, number of dummy cycles, ...) are
chosen once for all by spi_nor_init_params() then the actual memory device
is configured by spi_nor_setup().

I can rework spi_nor_setup() a little bit as the first part of its code is
needed to be done only once from spi_nor_scan() whereas the final part
almost matches what's your "spi_nor_reset()" function will do in term.

We should regroup {unlock sectors, enable_quad_io() and set_4byte() (if not
set_nor_set_4byte_opcodes)} codes. That is to say, the minimum set of SPI
commands we have to send to the memory at each power on.

Best regards,

Cyrille


> -		} else
> -			set_4byte(nor, info, 1);
> +		}
>  	} else {
>  		nor->addr_width = 3;
>  	}
> @@ -1517,6 +1533,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  
>  	nor->read_dummy = spi_nor_read_dummy_cycles(nor);
>  
> +	ret = spi_nor_reset(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 c425c7b..4733c04 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -121,6 +121,8 @@ enum spi_nor_option_flags {
>  	SNOR_F_HAS_SR_TB	= BIT(1),
>  };
>  
> +struct flash_info;
> +
>  /**
>   * struct spi_nor - Structure for defining a the SPI NOR layer
>   * @mtd:		point to a mtd_info structure
> @@ -154,6 +156,7 @@ enum spi_nor_option_flags {
>   * @priv:		the private data
>   */
>  struct spi_nor {
> +	const struct flash_info *info;
>  	struct mtd_info		mtd;
>  	struct mutex		lock;
>  	struct device		*dev;
> @@ -198,12 +201,28 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
>  }
>  
>  /**
> + * spi_nor_reset() - reset scan the SPI NOR
> + * @nor:	the spi_nor structure
> + *
> + * The drivers uses this function to reset the SPI NOR flash device to
> + * its initial scanned state, it shall use all nor information set on poweron
> + * for the read mode, address width and enabling write mode for certain
> + * manufacturers. This would be needed to be called for flash devices that are
> + * reset during power management.
> + *
> + * The chip type name can be provided through the @name parameter.
> + *
> + * Return: 0 for success, others for failure.
> + */
> +int spi_nor_reset(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{}.
>   *
> 


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

Re: [PATCH v2 1/2] mtd: spi-nor: Added spi-nor reset function

[Kamal Dasu]

On Thu, Feb 9, 2017 at 12:49 PM, Cyrille Pitchen
<cyrille.pitchen@atmel.com> wrote:
> Hi Kamal,
>
> Le 08/02/2017 à 17:53, Kamal Dasu a écrit :
>> Refactored spi_nor_scan() code to add spi_nor_reset() function that
>> programs the nand device 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
>>
>> On pm resume spi-nor flash may need to be reconfigured after power
>> reset, there is no need to go through a full spi_nor_scan(), flash
>> device driver needs to call spi_nor_reset() to reprogram the flash
>> to its probed state.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 62 ++++++++++++++++++++++++++++---------------
>>  include/linux/mtd/spi-nor.h   | 21 ++++++++++++++-
>>  2 files changed, 61 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index da7cd69..8e3d8bd 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1312,6 +1312,41 @@ static int spi_nor_check(struct spi_nor *nor)
>>       return 0;
>>  }
>>
>> +int spi_nor_reset(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 (nor->addr_width == 4 && JEDEC_MFR(info) != SNOR_MFR_SPANSION)
>> +             set_4byte(nor, info, 1);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_nor_reset);
>> +
>>  int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  {
>>       const struct flash_info *info = NULL;
>> @@ -1357,22 +1392,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>               }
>>       }
>>
>> +     nor->info = info;
>>       mutex_init(&nor->lock);
>>
>> -     /*
>> -      * 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;
>> @@ -1447,11 +1469,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;
>> @@ -1503,8 +1520,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>                       /* No small sector erase for 4-byte command set */
>>                       nor->erase_opcode = SPINOR_OP_SE_4B;
>>                       mtd->erasesize = info->sector_size;
>
> It seems that you didn't base your patch on the spi-nor tree. Before your
> patch, you should have something like:

I rebased to the linus tree. I will rebase to the spi-nor tree for the
next version.

>
>         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 {
>                 nor->addr_width = 3;
>         }
>
> That's why in my comment of review 1, I was talking about handling the
> case where spi_nor_set_4byte_opcodes() is called instead of set_4byte():
> indeed when the manufacturer id is Spansion or when the SPI_NOR_4B_OPCODES
> flag is set.
>

Ok  I need to use the spi-nor tree.

> Hence in spi_nor_reset(), or whatever name you choose, set_4byte() must not
> be called if the SPI_NOR_4B_OPCODES flag is set in info->flags.
>
> Besides, for now I don't know that much about how power management is
> handled by the Linux kernel, especially which events trigger the call of
> .suspend() or .resume().
> I've started to read documentation about that topic so please be patient
> because I need some time to figure out the potential side effects this
> series may introduce.
> I know you test your patches before submitting them, so this is not a
> question of trust but I'd rather understand what patches actually do
> instead of blindly merge them! :)
>

I will make sure I use the right codebase.

> Also maybe this patch can help you:
> http://patchwork.ozlabs.org/patch/719777/
>

I do see that the m25p80 and spi-nor drivers are implementing and
distinguishing spi protocol and cmds based on the address width and
transfer modes. And is spi-nor refactoring the same parts of the code
which I have touched. so will follow up in this change.

> It's not related to power management but it also reworks spi_nor_scan()
> so all settings (op codes, protocols, number of dummy cycles, ...) are
> chosen once for all by spi_nor_init_params() then the actual memory device
> is configured by spi_nor_setup().
>
> I can rework spi_nor_setup() a little bit as the first part of its code is
> needed to be done only once from spi_nor_scan() whereas the final part
> almost matches what's your "spi_nor_reset()" function will do in term.
>
> We should regroup {unlock sectors, enable_quad_io() and set_4byte() (if not
> set_nor_set_4byte_opcodes)} codes. That is to say, the minimum set of SPI
> commands we have to send to the memory at each power on.
>

Yes that sounds right. Regarding the pm  do we want to use the m25p80
resume, or let the spi master somehow invoke the setup from its
resume. If there was a way to do that cleanly within the current
framework of m25p, spi-nor, spi master it might leave that decision to
the spi controller driver. Also in our case we are a spi master
controller driver  mtd and spi-nor m25p80 structures and its ops are
transparent.

> Best regards,
>
> Cyrille
>
>
>> -             } else
>> -                     set_4byte(nor, info, 1);
>> +             }
>>       } else {
>>               nor->addr_width = 3;
>>       }
>> @@ -1517,6 +1533,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>
>>       nor->read_dummy = spi_nor_read_dummy_cycles(nor);
>>
>> +     ret = spi_nor_reset(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 c425c7b..4733c04 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -121,6 +121,8 @@ enum spi_nor_option_flags {
>>       SNOR_F_HAS_SR_TB        = BIT(1),
>>  };
>>
>> +struct flash_info;
>> +
>>  /**
>>   * struct spi_nor - Structure for defining a the SPI NOR layer
>>   * @mtd:             point to a mtd_info structure
>> @@ -154,6 +156,7 @@ enum spi_nor_option_flags {
>>   * @priv:            the private data
>>   */
>>  struct spi_nor {
>> +     const struct flash_info *info;
>>       struct mtd_info         mtd;
>>       struct mutex            lock;
>>       struct device           *dev;
>> @@ -198,12 +201,28 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
>>  }
>>
>>  /**
>> + * spi_nor_reset() - reset scan the SPI NOR
>> + * @nor:     the spi_nor structure
>> + *
>> + * The drivers uses this function to reset the SPI NOR flash device to
>> + * its initial scanned state, it shall use all nor information set on poweron
>> + * for the read mode, address width and enabling write mode for certain
>> + * manufacturers. This would be needed to be called for flash devices that are
>> + * reset during power management.
>> + *
>> + * The chip type name can be provided through the @name parameter.
>> + *
>> + * Return: 0 for success, others for failure.
>> + */
>> +int spi_nor_reset(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{}.
>>   *
>>
>

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