Re: [PATCH 4/8] mmc: mmci: Use ios_handler to save power

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Russell,

Sorry for a late reply.

On 04/18/2012 03:45 PM, Russell King - ARM Linux wrote:
On Wed, Apr 18, 2012 at 01:57:27PM +0200, Vitaly Wool wrote:
Hi Ulf,

On Tue, Jan 17, 2012 at 3:34 PM, Ulf Hansson<ulf.hansson@xxxxxxxxxxxxxx>  wrote:
To disable a levelshifter when we are in an idle state will
decrease current consumption. We make use of the ios_handler
at runtime suspend and at regular suspend to accomplish this.

Of course depending on the used levelshifter the decrease of
current differs. For ST6G3244ME the value is up to ~200 uA.

Tested-by: Linus Walleij<linus.walleij@xxxxxxxxxx>
Signed-off-by: Ulf Hansson<ulf.hansson@xxxxxxxxxxxxxx>
Signed-off-by: Linus Walleij<linus.walleij@xxxxxxxxxx>
---
  drivers/mmc/host/mmci.c |   19 ++++++++++++++++++-
  1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index a7c8f8f..76ce2cd 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1505,10 +1505,22 @@ static int mmci_save(struct amba_device *dev)
  {
        struct mmc_host *mmc = amba_get_drvdata(dev);
        unsigned long flags;
+       struct mmc_ios ios;
+       int ret = 0;

        if (mmc) {
                struct mmci_host *host = mmc_priv(mmc);

+               /* Let the ios_handler act on a POWER_OFF to save power. */
+               if (host->plat->ios_handler) {
+                       memcpy(&ios,&mmc->ios, sizeof(struct mmc_ios));
+                       ios.power_mode = MMC_POWER_OFF;
+                       ret = host->plat->ios_handler(mmc_dev(mmc),
+&ios);
+                       if (ret)
+                               return ret;
+               }
+
                spin_lock_irqsave(&host->lock, flags);

                /*
@@ -1527,7 +1539,7 @@ static int mmci_save(struct amba_device *dev)
                amba_vcore_disable(dev);
        }

-       return 0;
+       return ret;
  }

  static int mmci_restore(struct amba_device *dev)
@@ -1550,6 +1562,11 @@ static int mmci_restore(struct amba_device *dev)
                writel(MCI_IRQENABLE, host->base + MMCIMASK0);

                spin_unlock_irqrestore(&host->lock, flags);
+
+               /* Restore settings done by the ios_handler. */
+               if (host->plat->ios_handler)
+                       host->plat->ios_handler(mmc_dev(mmc),
+&mmc->ios);
        }

        return 0;

this patch is a disaster because it cuts off the chip's power on
suspend regardless of whether MMC_PM_KEEP_POWER flag has been set or
not. Simply put: it breaks everything This patch therefore gets a
strongest *NACK* from me.

Agree, the ios_handler should not be called like this. In our internal code base the ios_handler were only handling the levelshifter, which is safe to disable in the runtim_suspend sequence. But of course, and ios_handler may control power to the card as well and thus shall not be called like this patch does.


Thank you for backing up what I've already said about this patch (which
is why I stopped applying Ulf's MMC patches at this patch.)

I've also been concerned with the mere saving and restoring of the
clock and power registers in this patch - something which the ARM version
of the primecell does not actually support.

The ios_handler is not used for the ARM version so it should not be causing any issue I think. But, still, according to input from Vitaly above, this patch is not OK.

I will rework this patch. Thanks for your input.


I've said that before...


Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux