[PATCH] mtd: m25p80: Add Power Management support

[PATCH] mtd: m25p80: Add Power Management support

[Zhiqiang Hou]

From: Hou Zhiqiang <B48286@freescale.com>

Add the rescanning and initialization of SPI flash, to make the SPI
flash in the correct state. Because if the Power Management system
truns off power supply for SPI flash when system suspending, the SPI
flash will return to the reset state after system resume.

Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
---
 drivers/mtd/devices/m25p80.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d313f948b..f9d2b2e 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -248,6 +248,39 @@ static int m25p_remove(struct spi_device *spi)
 	return mtd_device_unregister(&flash->mtd);
 }
 
+#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);
+	struct spi_device *spi = flash->spi;
+	struct spi_nor *nor = &flash->spi_nor;
+	enum read_mode mode = SPI_NOR_NORMAL;
+	struct flash_platform_data *data;
+	char *flash_name = NULL;
+
+	if (spi->mode & SPI_RX_QUAD)
+		mode = SPI_NOR_QUAD;
+	else if (spi->mode & SPI_RX_DUAL)
+		mode = SPI_NOR_DUAL;
+
+	data = dev_get_platdata(&spi->dev);
+	if (data && data->type)
+		flash_name = data->type;
+	else if (!strcmp(spi->modalias, "spi-nor"))
+		flash_name = NULL; /* auto-detect */
+	else
+		flash_name = spi->modalias;
+
+	return spi_nor_scan(nor, flash_name, mode);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
 /*
  * Do NOT add to this array without reading the following:
  *
@@ -302,6 +335,7 @@ static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.owner	= THIS_MODULE,
+		.pm	= &m25p_pm_ops,
 	},
 	.id_table	= m25p_ids,
 	.probe	= m25p_probe,
-- 
2.1.0.27.g96db324

RE: [PATCH] mtd: m25p80: Add Power Management support

[Hou Zhiqiang]

Hi Brian,

Do you have any comment?

> -----Original Message-----
> From: Zhiqiang Hou [mailto:B48286@freescale.com]
> Sent: 2015年7月21日 18:19
> To: linux-mtd@lists.infradead.org; computersforpeace@gmail.com;
> dwmw2@infradead.org
> Cc: Hu Mingkai-B21284; Hou Zhiqiang-B48286
> Subject: [PATCH] mtd: m25p80: Add Power Management support
> 
> From: Hou Zhiqiang <B48286@freescale.com>
> 
> Add the rescanning and initialization of SPI flash, to make the SPI flash
> in the correct state. Because if the Power Management system truns off
> power supply for SPI flash when system suspending, the SPI flash will
> return to the reset state after system resume.
> 
> Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index d313f948b..f9d2b2e 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -248,6 +248,39 @@ static int m25p_remove(struct spi_device *spi)
>  	return mtd_device_unregister(&flash->mtd);
>  }
> 
> +#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);
> +	struct spi_device *spi = flash->spi;
> +	struct spi_nor *nor = &flash->spi_nor;
> +	enum read_mode mode = SPI_NOR_NORMAL;
> +	struct flash_platform_data *data;
> +	char *flash_name = NULL;
> +
> +	if (spi->mode & SPI_RX_QUAD)
> +		mode = SPI_NOR_QUAD;
> +	else if (spi->mode & SPI_RX_DUAL)
> +		mode = SPI_NOR_DUAL;
> +
> +	data = dev_get_platdata(&spi->dev);
> +	if (data && data->type)
> +		flash_name = data->type;
> +	else if (!strcmp(spi->modalias, "spi-nor"))
> +		flash_name = NULL; /* auto-detect */
> +	else
> +		flash_name = spi->modalias;
> +
> +	return spi_nor_scan(nor, flash_name, mode); } #endif /*
> +CONFIG_PM_SLEEP */
> +
> +static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
>  /*
>   * Do NOT add to this array without reading the following:
>   *
> @@ -302,6 +335,7 @@ static struct spi_driver m25p80_driver = {
>  	.driver = {
>  		.name	= "m25p80",
>  		.owner	= THIS_MODULE,
> +		.pm	= &m25p_pm_ops,
>  	},
>  	.id_table	= m25p_ids,
>  	.probe	= m25p_probe,
> --
> 2.1.0.27.g96db324

Thanks,
Zhiqiang

RE: [PATCH] mtd: m25p80: Add Power Management support

[Hou Zhiqiang]

Hi Brian,

If you haven't any comment with this patch, could you please apply this patch?

> -----Original Message-----
> From: Hou Zhiqiang-B48286
> Sent: 2015年7月31日 18:45
> To: Hou Zhiqiang-B48286; linux-mtd@lists.infradead.org;
> computersforpeace@gmail.com; dwmw2@infradead.org
> Cc: Hu Mingkai-B21284
> Subject: RE: [PATCH] mtd: m25p80: Add Power Management support
> 
> Hi Brian,
> 
> Do you have any comment?
> 
> > -----Original Message-----
> > From: Zhiqiang Hou [mailto:B48286@freescale.com]
> > Sent: 2015年7月21日 18:19
> > To: linux-mtd@lists.infradead.org; computersforpeace@gmail.com;
> > dwmw2@infradead.org
> > Cc: Hu Mingkai-B21284; Hou Zhiqiang-B48286
> > Subject: [PATCH] mtd: m25p80: Add Power Management support
> >
> > From: Hou Zhiqiang <B48286@freescale.com>
> >
> > Add the rescanning and initialization of SPI flash, to make the SPI
> > flash in the correct state. Because if the Power Management system
> > truns off power supply for SPI flash when system suspending, the SPI
> > flash will return to the reset state after system resume.
> >
> > Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
> > ---
> >  drivers/mtd/devices/m25p80.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c
> > b/drivers/mtd/devices/m25p80.c index d313f948b..f9d2b2e 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -248,6 +248,39 @@ static int m25p_remove(struct spi_device *spi)
> >  	return mtd_device_unregister(&flash->mtd);
> >  }
> >
> > +#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);
> > +	struct spi_device *spi = flash->spi;
> > +	struct spi_nor *nor = &flash->spi_nor;
> > +	enum read_mode mode = SPI_NOR_NORMAL;
> > +	struct flash_platform_data *data;
> > +	char *flash_name = NULL;
> > +
> > +	if (spi->mode & SPI_RX_QUAD)
> > +		mode = SPI_NOR_QUAD;
> > +	else if (spi->mode & SPI_RX_DUAL)
> > +		mode = SPI_NOR_DUAL;
> > +
> > +	data = dev_get_platdata(&spi->dev);
> > +	if (data && data->type)
> > +		flash_name = data->type;
> > +	else if (!strcmp(spi->modalias, "spi-nor"))
> > +		flash_name = NULL; /* auto-detect */
> > +	else
> > +		flash_name = spi->modalias;
> > +
> > +	return spi_nor_scan(nor, flash_name, mode); } #endif /*
> > +CONFIG_PM_SLEEP */
> > +
> > +static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
> >  /*
> >   * Do NOT add to this array without reading the following:
> >   *
> > @@ -302,6 +335,7 @@ static struct spi_driver m25p80_driver = {
> >  	.driver = {
> >  		.name	= "m25p80",
> >  		.owner	= THIS_MODULE,
> > +		.pm	= &m25p_pm_ops,
> >  	},
> >  	.id_table	= m25p_ids,
> >  	.probe	= m25p_probe,
> > --
> > 2.1.0.27.g96db324
> 
> Thanks,
> Zhiqiang

Thanks,
Zhiqiang

Re: [PATCH] mtd: m25p80: Add Power Management support

[Brian Norris]

On Fri, Aug 07, 2015 at 04:18:30AM +0000, Hou Zhiqiang wrote:
> If you haven't any comment with this patch, could you please apply this patch?

No. There's a backlog of much longer than 3 weeks.

Re: [PATCH] mtd: m25p80: Add Power Management support

[Brian Norris]

On Tue, Jul 21, 2015 at 06:18:53PM +0800, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <B48286@freescale.com>
> 
> Add the rescanning and initialization of SPI flash, to make the SPI
> flash in the correct state. Because if the Power Management system
> truns off power supply for SPI flash when system suspending, the SPI
> flash will return to the reset state after system resume.
> 
> Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index d313f948b..f9d2b2e 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -248,6 +248,39 @@ static int m25p_remove(struct spi_device *spi)
>  	return mtd_device_unregister(&flash->mtd);
>  }
>  
> +#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);
> +	struct spi_device *spi = flash->spi;
> +	struct spi_nor *nor = &flash->spi_nor;
> +	enum read_mode mode = SPI_NOR_NORMAL;
> +	struct flash_platform_data *data;
> +	char *flash_name = NULL;
> +
> +	if (spi->mode & SPI_RX_QUAD)
> +		mode = SPI_NOR_QUAD;
> +	else if (spi->mode & SPI_RX_DUAL)
> +		mode = SPI_NOR_DUAL;
> +
> +	data = dev_get_platdata(&spi->dev);
> +	if (data && data->type)
> +		flash_name = data->type;
> +	else if (!strcmp(spi->modalias, "spi-nor"))
> +		flash_name = NULL; /* auto-detect */
> +	else
> +		flash_name = spi->modalias;
> +
> +	return spi_nor_scan(nor, flash_name, mode);

No, this is not good. You need to be much more targeted than just
rerunning practically the entire driver init. Please introduce a
spi_nor_suspend() and spi_nor_resume() function, and have them do only
the steps that are necessary. e.g., re-set any flash modes, like 4-byte
addressing, etc. Then you can call them from the appropriate drivers,
like m25p80.c.

> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
>  /*
>   * Do NOT add to this array without reading the following:
>   *
> @@ -302,6 +335,7 @@ static struct spi_driver m25p80_driver = {
>  	.driver = {
>  		.name	= "m25p80",
>  		.owner	= THIS_MODULE,
> +		.pm	= &m25p_pm_ops,
>  	},
>  	.id_table	= m25p_ids,
>  	.probe	= m25p_probe,

Brian

[PATCH] mtd: m25p80: Add Power Management support

[Hou Zhiqiang]

Add PM support using callback function suspend and resume in .driver of
spi_driver.

Signed-off-by: Hou Zhiqiang <b48286@freescale.com>
---
 drivers/mtd/devices/m25p80.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7eda71d..b0c2b8c 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -66,6 +66,8 @@
 
 /* Used for Spansion flashes only. */
 #define	OPCODE_BRWR		0x17	/* Bank register write */
+#define	OPCODE_DP		0xb9	/* Enter deep power down mode */
+#define	OPCODE_RES		0xab	/* Exit deep power down mode */
 
 /* Status Register bits. */
 #define	SR_WIP			1	/* Write in progress */
@@ -1128,11 +1130,46 @@ static int m25p_remove(struct spi_device *spi)
 	return mtd_device_unregister(&flash->mtd);
 }
 
+#ifdef CONFIG_PM
+static int m25p_suspend(struct device *dev, pm_message_t mesg)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+	int ret;
+
+	flash->command[0] = OPCODE_DP;
+	mutex_lock(&flash->lock);
+	/* Wait until finished previous write/erase command. */
+	ret = wait_till_ready(flash);
+	if (ret) {
+		mutex_unlock(&flash->lock);
+		return ret;
+	}
+	ret = spi_write(flash->spi, flash->command, 1);
+	mutex_unlock(&flash->lock);
+
+	return ret;
+}
+
+static int m25p_resume(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+	int ret;
+
+	flash->command[0] = OPCODE_RES;
+	ret = spi_write(flash->spi, flash->command, 1);
+
+	return ret;
+}
+#endif /* CONFIG_PM */
 
 static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.suspend = m25p_suspend,
+		.resume = m25p_resume,
+#endif
 	},
 	.id_table	= m25p_ids,
 	.probe	= m25p_probe,
-- 
1.8.4.1

Re: [PATCH] mtd: m25p80: Add Power Management support

[Brian Norris]

On Wed, Dec 11, 2013 at 04:19:30PM +0800, Hou Zhiqiang wrote:
> Add PM support using callback function suspend and resume in .driver of
> spi_driver.
> 
> Signed-off-by: Hou Zhiqiang <b48286@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..b0c2b8c 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -66,6 +66,8 @@
>  
>  /* Used for Spansion flashes only. */
>  #define	OPCODE_BRWR		0x17	/* Bank register write */
> +#define	OPCODE_DP		0xb9	/* Enter deep power down mode */
> +#define	OPCODE_RES		0xab	/* Exit deep power down mode */

Where did you get these opcodes from? They are not in the Spansion
datasheets I'm reading. And in fact, they are overloaded as RES (Read
Electronic Signature, 0xab) and Bank Register Access (0xb9) in the
datasheet I'm looking at. So this patch is wrong.

Also, can you describe the purpose of these "deep power down" modes?
I've never seen PM states where the *flash* needs to be put into a lower
power mode. Typically the flash is pretty low-power when idle, and it
may even be powered off completely when the system enters a lower-power
state. Anyway, please describe why this patch is needed.

>  
>  /* Status Register bits. */
>  #define	SR_WIP			1	/* Write in progress */
> @@ -1128,11 +1130,46 @@ static int m25p_remove(struct spi_device *spi)
>  	return mtd_device_unregister(&flash->mtd);
>  }
>  
> +#ifdef CONFIG_PM
> +static int m25p_suspend(struct device *dev, pm_message_t mesg)
> +{
> +	struct m25p *flash = dev_get_drvdata(dev);
> +	int ret;
> +
> +	flash->command[0] = OPCODE_DP;

As mentioned above, this opcode is not recognized by many flash
supported in this driver. So we might want one or more of the following:

 (1) to assign different suspend/resume opcodes for use in
     m25p_suspend/resume
 (2) to provide over-loadable callbacks so that different flash could
     use different suspend/resume routines

And of course, we need to avoid sending these commands at all to
unsupported flash.

> +	mutex_lock(&flash->lock);
> +	/* Wait until finished previous write/erase command. */
> +	ret = wait_till_ready(flash);
> +	if (ret) {
> +		mutex_unlock(&flash->lock);
> +		return ret;
> +	}
> +	ret = spi_write(flash->spi, flash->command, 1);
> +	mutex_unlock(&flash->lock);
> +
> +	return ret;
> +}
> +
> +static int m25p_resume(struct device *dev)
> +{
> +	struct m25p *flash = dev_get_drvdata(dev);
> +	int ret;
> +
> +	flash->command[0] = OPCODE_RES;
> +	ret = spi_write(flash->spi, flash->command, 1);
> +
> +	return ret;
> +}
> +#endif /* CONFIG_PM */
>  
>  static struct spi_driver m25p80_driver = {
>  	.driver = {
>  		.name	= "m25p80",
>  		.owner	= THIS_MODULE,
> +#ifdef CONFIG_PM
> +		.suspend = m25p_suspend,
> +		.resume = m25p_resume,
> +#endif
>  	},
>  	.id_table	= m25p_ids,
>  	.probe	= m25p_probe,

Brian

RE: [PATCH] mtd: m25p80: Add Power Management support

[B48286]

>On Wed, Dec 11, 2013 at 04:19:30PM +0800, Hou Zhiqiang wrote:
>> Add PM support using callback function suspend and resume in .driver 
>> of spi_driver.
>> 
>> Signed-off-by: Hou Zhiqiang <b48286@freescale.com>
>> ---
>>  drivers/mtd/devices/m25p80.c | 37 
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>> 
>> diff --git a/drivers/mtd/devices/m25p80.c 
>> b/drivers/mtd/devices/m25p80.c index 7eda71d..b0c2b8c 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -66,6 +66,8 @@
>>  
>>  /* Used for Spansion flashes only. */
>>  #define	OPCODE_BRWR		0x17	/* Bank register write */
>> +#define	OPCODE_DP		0xb9	/* Enter deep power down mode */
>> +#define	OPCODE_RES		0xab	/* Exit deep power down mode */
>
>Where did you get these opcodes from? They are not in the Spansion datasheets I'm reading. And in fact, they are overloaded as RES (Read Electronic Signature, 0xab) and Bank Register Access (0xb9) in the datasheet I'm looking at. So this patch is wrong.
>

In datasheet S25FL128P, Deep Power Down command is b9h and Release from Deep Power Down command is abh. In S25FL-A to S25FL-P Migration Guide those commands are the same.

>Also, can you describe the purpose of these "deep power down" modes?
>I've never seen PM states where the *flash* needs to be put into a lower power mode. Typically the flash is pretty low-power when idle, and it may even be powered off completely when the system enters a lower-power state. Anyway, please describe why this patch is needed.
>

In standby mode, the MAX currunt consumption is 200mA, and in Deep Power Down mode, the MAX is 20mA. In actually the typically value of currunt consumption is 3mA, so it save power consumption significantly I think.

>>  
>>  /* Status Register bits. */
>>  #define	SR_WIP			1	/* Write in progress */
>> @@ -1128,11 +1130,46 @@ static int m25p_remove(struct spi_device *spi)
>>  	return mtd_device_unregister(&flash->mtd);
>>  }
>>  
>> +#ifdef CONFIG_PM
>> +static int m25p_suspend(struct device *dev, pm_message_t mesg) {
>> +	struct m25p *flash = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	flash->command[0] = OPCODE_DP;
>
>As mentioned above, this opcode is not recognized by many flash supported in this driver. So we might want one or more of the following:
>
> (1) to assign different suspend/resume opcodes for use in
>     m25p_suspend/resume
> (2) to provide over-loadable callbacks so that different flash could
>     use different suspend/resume routines
>
>And of course, we need to avoid sending these commands at all to unsupported flash.
>

Yeah, in m25p_probe we can get spi flash specified PM commands from somewhere, but where can we set the PM commands, in struct spi_device_id?
Do you have some good suggestion?

>> +	mutex_lock(&flash->lock);
>> +	/* Wait until finished previous write/erase command. */
>> +	ret = wait_till_ready(flash);
>> +	if (ret) {
>> +		mutex_unlock(&flash->lock);
>> +		return ret;
>> +	}
>> +	ret = spi_write(flash->spi, flash->command, 1);
>> +	mutex_unlock(&flash->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int m25p_resume(struct device *dev) {
>> +	struct m25p *flash = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	flash->command[0] = OPCODE_RES;
>> +	ret = spi_write(flash->spi, flash->command, 1);
>> +
>> +	return ret;
>> +}
>> +#endif /* CONFIG_PM */
>>  
>>  static struct spi_driver m25p80_driver = {
>>  	.driver = {
>>  		.name	= "m25p80",
>>  		.owner	= THIS_MODULE,
>> +#ifdef CONFIG_PM
>> +		.suspend = m25p_suspend,
>> +		.resume = m25p_resume,
>> +#endif
>>  	},
>>  	.id_table	= m25p_ids,
>>  	.probe	= m25p_probe,
>
>Brian

Zhiqiang Hou