Patchwork [2/7] mtd: OneNAND: add enable / disable methods to onenand_chip

login
register
mail settings
Submitter Adrian Hunter
Date Dec. 13, 2010, 12:20 p.m.
Message ID <20101213122057.20685.68480.sendpatchset@ahunter-work.research.nokia.com>
Download mbox | patch
Permalink /patch/75326/
State New
Headers show

Comments

Adrian Hunter - Dec. 13, 2010, 12:20 p.m.
From 16b760999cb79f9cf71728c9253f1399717be63a Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@nokia.com>
Date: Fri, 19 Feb 2010 15:39:52 +0100
Subject: [PATCH 2/7] mtd: OneNAND: add enable / disable methods to onenand_chip

Add enable / disable methods called from get_device() / release_device().
These can be used, for example, to allow the driver to prevent the voltage
regulator from being put to sleep while OneNAND is in use.

Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |    4 ++++
 include/linux/mtd/onenand.h        |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)
Kyungmin Park - Dec. 14, 2010, 12:17 a.m.
Hi,

Now I'm used the clock gating for OneNAND instead of regulator.
so I think the mechanism is similar.

On Mon, Dec 13, 2010 at 9:20 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
> From 16b760999cb79f9cf71728c9253f1399717be63a Mon Sep 17 00:00:00 2001
> From: Adrian Hunter <adrian.hunter@nokia.com>
> Date: Fri, 19 Feb 2010 15:39:52 +0100
> Subject: [PATCH 2/7] mtd: OneNAND: add enable / disable methods to onenand_chip
>
> Add enable / disable methods called from get_device() / release_device().
> These can be used, for example, to allow the driver to prevent the voltage
> regulator from being put to sleep while OneNAND is in use.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
> ---
>  drivers/mtd/onenand/onenand_base.c |    4 ++++
>  include/linux/mtd/onenand.h        |    2 ++
>  2 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index c38bf9c..bde274f 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -948,6 +948,8 @@ static int onenand_get_device(struct mtd_info *mtd, int new_state)
>                if (this->state == FL_READY) {
>                        this->state = new_state;
>                        spin_unlock(&this->chip_lock);
> +                       if (this->enable)
> +                               this->enable(mtd);
>                        break;
>                }
>                if (new_state == FL_PM_SUSPENDED) {

I put the code the end of function like:

static int onenand_get_device(struct mtd_info *mtd, int new_state)
{
        struct onenand_chip *this = mtd->priv;
        DECLARE_WAITQUEUE(wait, current);

        /*
         * Grab the lock and see if the device is available
         */
        while (1) {
                spin_lock(&this->chip_lock);
                if (this->state == FL_READY) {
                        this->state = new_state;
                        spin_unlock(&this->chip_lock);
                        break;
                }
                if (new_state == FL_PM_SUSPENDED) {
                        spin_unlock(&this->chip_lock);
                        return (this->state == FL_PM_SUSPENDED) ? 0 : -EAGAIN;
                }
                set_current_state(TASK_UNINTERRUPTIBLE);
                add_wait_queue(&this->wq, &wait);
                spin_unlock(&this->chip_lock);
                schedule();
                remove_wait_queue(&this->wq, &wait);
        }

        if (this->clk)
                clk_enable(this->clk);

        return 0;
}


> @@ -974,6 +976,8 @@ static void onenand_release_device(struct mtd_info *mtd)
>  {
>        struct onenand_chip *this = mtd->priv;
>
> +       if (this->state != FL_PM_SUSPENDED && this->disable)
> +               this->disable(mtd);
Need to check the SUSPENDED at here? when enter the suspend get_device
is called.
release device is called when resume. so no need to check
FL_PM_SUSPENDED in my case.

>        /* Release the chip */
>        spin_lock(&this->chip_lock);
>        this->state = FL_READY;

and put the same as like:

static void onenand_release_device(struct mtd_info *mtd)
{
        struct onenand_chip *this = mtd->priv;

        /* Release the chip */
        spin_lock(&this->chip_lock);
        this->state = FL_READY;
        wake_up(&this->wq);
        spin_unlock(&this->chip_lock);

        if (this->clk)
                clk_disable(this->clk);
}

Thank you,
Kyungmin Park

> diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
> index 6da3fe3..ae418e4 100644
> --- a/include/linux/mtd/onenand.h
> +++ b/include/linux/mtd/onenand.h
> @@ -118,6 +118,8 @@ struct onenand_chip {
>        int (*chip_probe)(struct mtd_info *mtd);
>        int (*block_markbad)(struct mtd_info *mtd, loff_t ofs);
>        int (*scan_bbt)(struct mtd_info *mtd);
> +       int (*enable)(struct mtd_info *mtd);
> +       int (*disable)(struct mtd_info *mtd);
>
>        struct completion       complete;
>        int                     irq;
> --
> 1.7.0.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Adrian Hunter - Dec. 15, 2010, 7:31 a.m.
On 14/12/10 02:17, Kyungmin Park wrote:
> Hi,
>
> Now I'm used the clock gating for OneNAND instead of regulator.
> so I think the mechanism is similar.

I am not sure what you are trying to say here.  We need the regulator
enable / disable to stop the regulator going to sleep (because it cannot
supply enough current when it is asleep) and also we need to call down to
the board level to set latency constraints.  These things seem pretty
specific to the OMAP driver hence the new enable() / disable() methods.

We do not need any clock gating.  The OneNAND clock is dealt with by
the OMAP GPMC OneNAND controller and GPMC has auto-idle / smart-idle
facilities that let OMAP gate clocks and power it off when it is not
being used.

>
> On Mon, Dec 13, 2010 at 9:20 PM, Adrian Hunter<adrian.hunter@nokia.com>  wrote:
>>  From 16b760999cb79f9cf71728c9253f1399717be63a Mon Sep 17 00:00:00 2001
>> From: Adrian Hunter<adrian.hunter@nokia.com>
>> Date: Fri, 19 Feb 2010 15:39:52 +0100
>> Subject: [PATCH 2/7] mtd: OneNAND: add enable / disable methods to onenand_chip
>>
>> Add enable / disable methods called from get_device() / release_device().
>> These can be used, for example, to allow the driver to prevent the voltage
>> regulator from being put to sleep while OneNAND is in use.
>>
>> Signed-off-by: Adrian Hunter<adrian.hunter@nokia.com>
>> ---
>>   drivers/mtd/onenand/onenand_base.c |    4 ++++
>>   include/linux/mtd/onenand.h        |    2 ++
>>   2 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
>> index c38bf9c..bde274f 100644
>> --- a/drivers/mtd/onenand/onenand_base.c
>> +++ b/drivers/mtd/onenand/onenand_base.c
>> @@ -948,6 +948,8 @@ static int onenand_get_device(struct mtd_info *mtd, int new_state)
>>                 if (this->state == FL_READY) {
>>                         this->state = new_state;
>>                         spin_unlock(&this->chip_lock);
>> +                       if (this->enable)
>> +                               this->enable(mtd);
>>                         break;
>>                 }
>>                 if (new_state == FL_PM_SUSPENDED) {
>
> I put the code the end of function like:
>
> static int onenand_get_device(struct mtd_info *mtd, int new_state)
> {
>          struct onenand_chip *this = mtd->priv;
>          DECLARE_WAITQUEUE(wait, current);
>
>          /*
>           * Grab the lock and see if the device is available
>           */
>          while (1) {
>                  spin_lock(&this->chip_lock);
>                  if (this->state == FL_READY) {
>                          this->state = new_state;
>                          spin_unlock(&this->chip_lock);
>                          break;
>                  }
>                  if (new_state == FL_PM_SUSPENDED) {
>                          spin_unlock(&this->chip_lock);
>                          return (this->state == FL_PM_SUSPENDED) ? 0 : -EAGAIN;
>                  }
>                  set_current_state(TASK_UNINTERRUPTIBLE);
>                  add_wait_queue(&this->wq,&wait);
>                  spin_unlock(&this->chip_lock);
>                  schedule();
>                  remove_wait_queue(&this->wq,&wait);
>          }
>
>          if (this->clk)
>                  clk_enable(this->clk);
>
>          return 0;
> }
>
>
>> @@ -974,6 +976,8 @@ static void onenand_release_device(struct mtd_info *mtd)
>>   {
>>         struct onenand_chip *this = mtd->priv;
>>
>> +       if (this->state != FL_PM_SUSPENDED&&  this->disable)
>> +               this->disable(mtd);
> Need to check the SUSPENDED at here? when enter the suspend get_device
> is called.
> release device is called when resume. so no need to check
> FL_PM_SUSPENDED in my case.

onenand-get_device should have had:

	if (new_state != FL_PM_SUSPENDED && this->enable)
		this->enable(mtd);

I will resend the patch.


>
>>         /* Release the chip */
>>         spin_lock(&this->chip_lock);
>>         this->state = FL_READY;
>
> and put the same as like:
>
> static void onenand_release_device(struct mtd_info *mtd)
> {
>          struct onenand_chip *this = mtd->priv;
>
>          /* Release the chip */
>          spin_lock(&this->chip_lock);
>          this->state = FL_READY;
>          wake_up(&this->wq);
>          spin_unlock(&this->chip_lock);
>
>          if (this->clk)
>                  clk_disable(this->clk);
> }
>
> Thank you,
> Kyungmin Park
>
>> diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
>> index 6da3fe3..ae418e4 100644
>> --- a/include/linux/mtd/onenand.h
>> +++ b/include/linux/mtd/onenand.h
>> @@ -118,6 +118,8 @@ struct onenand_chip {
>>         int (*chip_probe)(struct mtd_info *mtd);
>>         int (*block_markbad)(struct mtd_info *mtd, loff_t ofs);
>>         int (*scan_bbt)(struct mtd_info *mtd);
>> +       int (*enable)(struct mtd_info *mtd);
>> +       int (*disable)(struct mtd_info *mtd);
>>
>>         struct completion       complete;
>>         int                     irq;
>> --
>> 1.7.0.4
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>

Patch

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index c38bf9c..bde274f 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -948,6 +948,8 @@  static int onenand_get_device(struct mtd_info *mtd, int new_state)
 		if (this->state == FL_READY) {
 			this->state = new_state;
 			spin_unlock(&this->chip_lock);
+			if (this->enable)
+				this->enable(mtd);
 			break;
 		}
 		if (new_state == FL_PM_SUSPENDED) {
@@ -974,6 +976,8 @@  static void onenand_release_device(struct mtd_info *mtd)
 {
 	struct onenand_chip *this = mtd->priv;
 
+	if (this->state != FL_PM_SUSPENDED && this->disable)
+		this->disable(mtd);
 	/* Release the chip */
 	spin_lock(&this->chip_lock);
 	this->state = FL_READY;
diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
index 6da3fe3..ae418e4 100644
--- a/include/linux/mtd/onenand.h
+++ b/include/linux/mtd/onenand.h
@@ -118,6 +118,8 @@  struct onenand_chip {
 	int (*chip_probe)(struct mtd_info *mtd);
 	int (*block_markbad)(struct mtd_info *mtd, loff_t ofs);
 	int (*scan_bbt)(struct mtd_info *mtd);
+	int (*enable)(struct mtd_info *mtd);
+	int (*disable)(struct mtd_info *mtd);
 
 	struct completion	complete;
 	int			irq;