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:

quoted

Hi Ulf,

On Tue, Jan 17, 2012 at 3:34 PM, Ulf Hansson[off-list ref]  wrote:

quoted

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<redacted>
Signed-off-by: Ulf Hansson<redacted>
Signed-off-by: Linus Walleij<redacted>
---
  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