[PATCH v2 0/4] suspend/resume support for OMAP nand driver

[PATCH v2 0/4] suspend/resume support for OMAP nand driver

[Philip Avinash]

This patch series adds low power transition support for OMAP NAND driver.
With recent driver conversion of GPMC to platform_driver, pm_runtime calls
can be used to handle module enable & disable of GPMC. This is taken care
patch #1.

patch #2 is for GPMC suspend/resume support.

This includes low power transition support of
- GPMC module
- ELM module
- OMAP2 NAND driver

User initiated suspend/resume sequence tested on am335x-evm with NAND
flash support. Also tested in omap3-evm for user initiated
suspend/resume sequence with NAND flash.
This patch series based on [1] and is present in [2].

1. ARM: OMAP2+: AM33XX: Add suspend-resume support
   http://comments.gmane.org/gmane.linux.ports.arm.omap/91405
2. https://github.com/avinashphilip/am335x_linux/commits/NAND_supend_resume_support


Philip Avinash (4):
  arm: gpmc: Converts GPMC driver to pm_runtime capable
  arm: gpmc: Low power transition support
  mtd: devices: elm: Low power transition support
  mtd: nand: omap2: Low power transition support

 arch/arm/mach-omap2/gpmc.c |   30 +++++++++++++++++++++++++++---
 drivers/mtd/devices/elm.c  |   40 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/omap2.c   |   19 +++++++++++++++++++
 3 files changed, 86 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] arm: gpmc: Converts GPMC driver to pm_runtime capable

[Philip Avinash]

Support for pm_runtime add to GPMC driver.

Signed-off-by: Philip Avinash <avinashphilip@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 2c57f81..b1cd6c1 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -29,6 +29,7 @@
 #include <linux/of_mtd.h>
 #include <linux/of_device.h>
 #include <linux/mtd/nand.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/platform_data/mtd-nand-omap2.h>
 
@@ -1317,7 +1318,8 @@ static int gpmc_probe(struct platform_device *pdev)
 		return PTR_ERR(gpmc_l3_clk);
 	}
 
-	clk_prepare_enable(gpmc_l3_clk);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
 
 	gpmc_dev = &pdev->dev;
 
@@ -1329,7 +1331,7 @@ static int gpmc_probe(struct platform_device *pdev)
 
 	rc = gpmc_mem_init();
 	if (IS_ERR_VALUE(rc)) {
-		clk_disable_unprepare(gpmc_l3_clk);
+		pm_runtime_put_sync(&pdev->dev);
 		clk_put(gpmc_l3_clk);
 		dev_err(gpmc_dev, "failed to reserve memory\n");
 		return rc;
@@ -1340,7 +1342,7 @@ static int gpmc_probe(struct platform_device *pdev)
 
 	rc = gpmc_probe_dt(pdev);
 	if (rc < 0) {
-		clk_disable_unprepare(gpmc_l3_clk);
+		pm_runtime_put_sync(&pdev->dev);
 		clk_put(gpmc_l3_clk);
 		dev_err(gpmc_dev, "failed to probe DT parameters\n");
 		return rc;
@@ -1353,6 +1355,8 @@ static __devexit int gpmc_remove(struct platform_device *pdev)
 {
 	gpmc_free_irq();
 	gpmc_mem_exit();
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	gpmc_dev = NULL;
 	return 0;
 }
-- 
1.7.9.5

[PATCH v2 2/4] arm: gpmc: Low power transition support

[Philip Avinash]

With GPMC converted to platform driver recently, adds low power
transition support in driver itself.

Signed-off-by: Philip Avinash <avinashphilip@ti.com>
---
Changes since v1:
	Module disable & enable added using pm_runtime support.

 arch/arm/mach-omap2/gpmc.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index b1cd6c1..cc57988 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -1361,9 +1361,29 @@ static __devexit int gpmc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int gpmc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	omap3_gpmc_save_context();
+	pm_runtime_put_sync(&pdev->dev);
+	return 0;
+}
+
+static int gpmc_resume(struct platform_device *pdev)
+{
+	pm_runtime_get_sync(&pdev->dev);
+	omap3_gpmc_restore_context();
+	return 0;
+}
+#endif
+
 static struct platform_driver gpmc_driver = {
 	.probe		= gpmc_probe,
 	.remove		= __devexit_p(gpmc_remove),
+#ifdef CONFIG_PM
+	.suspend	= gpmc_suspend,
+	.resume		= gpmc_resume,
+#endif
 	.driver		= {
 		.name	= DEVICE_NAME,
 		.owner	= THIS_MODULE,
-- 
1.7.9.5

[PATCH v2 3/4] mtd: devices: elm: Low power transition support

[Philip Avinash]

In low power modes of AM335X platforms, peripherals power is cut off.
This patch supports low power sleep transition support for ELM driver.

Signed-off-by: Philip Avinash <avinashphilip@ti.com>
---
 drivers/mtd/devices/elm.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c
index f034239..a9ac8f2 100644
--- a/drivers/mtd/devices/elm.c
+++ b/drivers/mtd/devices/elm.c
@@ -20,6 +20,7 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/sched.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_data/elm.h>
 
@@ -62,6 +63,7 @@ struct elm_info {
 	struct completion elm_completion;
 	struct list_head list;
 	enum bch_ecc bch_type;
+	bool idle;
 };
 
 static LIST_HEAD(elm_devices);
@@ -86,9 +88,11 @@ void elm_config(struct device *dev, enum bch_ecc bch_type)
 	u32 reg_val;
 	struct elm_info *info = dev_get_drvdata(dev);
 
+	info->idle = true;
 	reg_val = (bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16);
 	elm_write_reg(info, ELM_LOCATION_CONFIG, reg_val);
 	info->bch_type = bch_type;
+	info->idle = false;
 }
 EXPORT_SYMBOL(elm_config);
 
@@ -280,6 +284,7 @@ void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 	struct elm_info *info = dev_get_drvdata(dev);
 	u32 reg_val;
 
+	info->idle = true;
 	/* Enable page mode interrupt */
 	reg_val = elm_read_reg(info, ELM_IRQSTATUS);
 	elm_write_reg(info, ELM_IRQSTATUS, reg_val & INTR_STATUS_PAGE_VALID);
@@ -298,6 +303,7 @@ void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 	reg_val = elm_read_reg(info, ELM_IRQENABLE);
 	elm_write_reg(info, ELM_IRQENABLE, reg_val & ~INTR_EN_PAGE_MASK);
 	elm_error_correction(info, err_vec);
+	info->idle = false;
 }
 EXPORT_SYMBOL(elm_decode_bch_error_page);
 
@@ -368,6 +374,7 @@ static int elm_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&info->list);
 	list_add(&info->list, &elm_devices);
 	platform_set_drvdata(pdev, info);
+	info->idle = false;
 	return ret;
 }
 
@@ -379,6 +386,38 @@ static int elm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int elm_suspend(struct device *dev)
+{
+	struct elm_info *info = dev_get_drvdata(dev);
+	wait_queue_head_t wq;
+	DECLARE_WAITQUEUE(wait, current);
+
+	init_waitqueue_head(&wq);
+	while (1) {
+		/* Make sure that ELM not running */
+		if (info->idle) {
+			add_wait_queue(&wq, &wait);
+			schedule();
+			remove_wait_queue(&wq, &wait);
+		} else {
+			break;
+		}
+	}
+	pm_runtime_put_sync(dev);
+	return 0;
+}
+
+static int elm_resume(struct device *dev)
+{
+	struct elm_info *info = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+	elm_config(dev, info->bch_type);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(elm_pm_ops, elm_suspend, elm_resume);
+
 #ifdef CONFIG_OF
 static const struct of_device_id elm_of_match[] = {
 	{ .compatible = "ti,am3352-elm" },
@@ -392,6 +431,7 @@ static struct platform_driver elm_driver = {
 		.name	= "elm",
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(elm_of_match),
+		.pm	= &elm_pm_ops,
 	},
 	.probe	= elm_probe,
 	.remove	= elm_remove,
-- 
1.7.9.5

[PATCH v2 3/4] mtd: devices: elm: Low power transition support

[Russell King - ARM Linux]

On Thu, Feb 07, 2013 at 06:06:57PM +0530, Philip Avinash wrote:
> +static int elm_suspend(struct device *dev)
> +{
> +	struct elm_info *info = dev_get_drvdata(dev);
> +	wait_queue_head_t wq;
> +	DECLARE_WAITQUEUE(wait, current);
> +
> +	init_waitqueue_head(&wq);
> +	while (1) {
> +		/* Make sure that ELM not running */
> +		if (info->idle) {
> +			add_wait_queue(&wq, &wait);
> +			schedule();
> +			remove_wait_queue(&wq, &wait);
> +		} else {
> +			break;
> +		}
> +	}

The above code looks really wrong - it will just spin endlessly with the
waitqueues doing nothing useful.  What are you trying to do here?

[PATCH v2 3/4] mtd: devices: elm: Low power transition support

[Philip, Avinash]

On Sat, Feb 09, 2013 at 15:52:44, Russell King - ARM Linux wrote:
> On Thu, Feb 07, 2013 at 06:06:57PM +0530, Philip Avinash wrote:
> > +static int elm_suspend(struct device *dev)
> > +{
> > +	struct elm_info *info = dev_get_drvdata(dev);
> > +	wait_queue_head_t wq;
> > +	DECLARE_WAITQUEUE(wait, current);
> > +
> > +	init_waitqueue_head(&wq);
> > +	while (1) {
> > +		/* Make sure that ELM not running */
> > +		if (info->idle) {
> > +			add_wait_queue(&wq, &wait);
> > +			schedule();
> > +			remove_wait_queue(&wq, &wait);
> > +		} else {
> > +			break;
> > +		}
> > +	}
> 
> The above code looks really wrong - it will just spin endlessly with the
> waitqueues doing nothing useful.  What are you trying to do here?

The intention of waitqeue is to make the suspend process really wait till
the ELM module finishes current activity. Although this type of protection
already achieved in mtd layer using nand_suspend(), this one is particularly
required for ELM module to really make sure that *any pending* corrections to
finish really before gone to suspend.

If suspend activity initiated and NAND driver requested service of ELM
module for error correction, then suspend activity has to wait till
(info->idle == false), so that ELM module finishes activity.

I can think of adding different states to ELM module and take action.
I will add following states.

RUNNING
SUSPEND

Thanks
Avinash

> 

[PATCH v2 3/4] mtd: devices: elm: Low power transition support

[Russell King - ARM Linux]

On Wed, Feb 13, 2013 at 11:42:01AM +0000, Philip, Avinash wrote:
> On Sat, Feb 09, 2013 at 15:52:44, Russell King - ARM Linux wrote:
> > On Thu, Feb 07, 2013 at 06:06:57PM +0530, Philip Avinash wrote:
> > > +static int elm_suspend(struct device *dev)
> > > +{
> > > +	struct elm_info *info = dev_get_drvdata(dev);
> > > +	wait_queue_head_t wq;
> > > +	DECLARE_WAITQUEUE(wait, current);
> > > +
> > > +	init_waitqueue_head(&wq);
> > > +	while (1) {
> > > +		/* Make sure that ELM not running */
> > > +		if (info->idle) {
> > > +			add_wait_queue(&wq, &wait);
> > > +			schedule();
> > > +			remove_wait_queue(&wq, &wait);
> > > +		} else {
> > > +			break;
> > > +		}
> > > +	}
> > 
> > The above code looks really wrong - it will just spin endlessly with the
> > waitqueues doing nothing useful.  What are you trying to do here?
> 
> The intention of waitqeue is to make the suspend process really wait till
> the ELM module finishes current activity. Although this type of protection
> already achieved in mtd layer using nand_suspend(), this one is particularly
> required for ELM module to really make sure that *any pending* corrections to
> finish really before gone to suspend.

I don't think you understand what's going on with the above, and why the
above is completely ineffective.

1. Your wait queue head is declared on stack, and there's no references
   to it outside of this function.  So _nothing_ can activate the wait
   queue.
2. You're not changing the current thread's status away from TASK_RUNNING,
   so schedule() will either return immediately or it will schedule another
   task if it's time for this one to be preempted.

In other words, the above can be rewritten as:

	while (info->idle)
		schedule();

and it will still have the same effect.

Now, if you want to be kinder to the system, then you should use a
wait queue properly.  Put the waitqueue head in struct elm_info.  Use
wait_event() here.  And call wake_up() where you set info->idle to
false.

[PATCH v2 3/4] mtd: devices: elm: Low power transition support

[Philip, Avinash]

On Wed, Feb 13, 2013 at 18:13:03, Russell King - ARM Linux wrote:
> On Wed, Feb 13, 2013 at 11:42:01AM +0000, Philip, Avinash wrote:
> > On Sat, Feb 09, 2013 at 15:52:44, Russell King - ARM Linux wrote:
> > > On Thu, Feb 07, 2013 at 06:06:57PM +0530, Philip Avinash wrote:
> > > > +static int elm_suspend(struct device *dev)
> > > > +{
> > > > +	struct elm_info *info = dev_get_drvdata(dev);
> > > > +	wait_queue_head_t wq;
> > > > +	DECLARE_WAITQUEUE(wait, current);
> > > > +
> > > > +	init_waitqueue_head(&wq);
> > > > +	while (1) {
> > > > +		/* Make sure that ELM not running */
> > > > +		if (info->idle) {
> > > > +			add_wait_queue(&wq, &wait);
> > > > +			schedule();
> > > > +			remove_wait_queue(&wq, &wait);
> > > > +		} else {
> > > > +			break;
> > > > +		}
> > > > +	}
> > > 
> > > The above code looks really wrong - it will just spin endlessly with the
> > > waitqueues doing nothing useful.  What are you trying to do here?
> > 
> > The intention of waitqeue is to make the suspend process really wait till
> > the ELM module finishes current activity. Although this type of protection
> > already achieved in mtd layer using nand_suspend(), this one is particularly
> > required for ELM module to really make sure that *any pending* corrections to
> > finish really before gone to suspend.
> 
> I don't think you understand what's going on with the above, and why the
> above is completely ineffective.
> 
> 1. Your wait queue head is declared on stack, and there's no references
>    to it outside of this function.  So _nothing_ can activate the wait
>    queue.
> 2. You're not changing the current thread's status away from TASK_RUNNING,
>    so schedule() will either return immediately or it will schedule another
>    task if it's time for this one to be preempted.
> 
> In other words, the above can be rewritten as:
> 
> 	while (info->idle)
> 		schedule();
> 
> and it will still have the same effect.
> 
> Now, if you want to be kinder to the system, then you should use a
> wait queue properly.  Put the waitqueue head in struct elm_info.  Use
> wait_event() here.  And call wake_up() where you set info->idle to
> false.

I understood the issue. Thanks for the detailed explanation.

It seems the entire mechanism of ELM module state check is not required.
The ELM suspend procedure initiated only after the current MTD transaction
finishes and is ensured in MTD class driver.

So I can simply disable ELM module in suspend without any check.

Thanks
Avinash

> 

[PATCH v2 4/4] mtd: nand: omap2: Low power transition support

[Philip Avinash]

Add support for Low power transition support in nand driver. Also
ensures the current transaction finishes before going to low power mode
with _suspend support in mtd layer.

Signed-off-by: Philip Avinash <avinashphilip@ti.com>
---
 drivers/mtd/nand/omap2.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 8e820dd..790215b 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -2103,9 +2103,28 @@ static int omap_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int omap_nand_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct mtd_info *mtd = platform_get_drvdata(pdev);
+
+	mtd->_suspend(mtd);
+	return 0;
+}
+
+static int omap_nand_resume(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static struct platform_driver omap_nand_driver = {
 	.probe		= omap_nand_probe,
 	.remove		= omap_nand_remove,
+#ifdef CONFIG_PM
+	.suspend	= omap_nand_suspend,
+	.resume		= omap_nand_resume,
+#endif
 	.driver		= {
 		.name	= DRIVER_NAME,
 		.owner	= THIS_MODULE,
-- 
1.7.9.5