OMAP: HSMMC: Initialize hsmmc controller registers when resuming

Commit Message

Adrian Hunter Feb. 23, 2009, 1:47 p.m. UTC

Kyungmin Park wrote:
> Hi,
> 
> On Mon, Feb 23, 2009 at 5:04 PM, Adrian Hunter
> <[e-mail]> wrote:
>> ext Kim Kyuwon wrote:
>>> Hi,
>>>
>>> On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <[e-mail]> wrote:
>>>> On Friday 20 February 2009, Kim Kyuwon wrote:
>>>>> +static void omap_hsmmc_init(struct mmc_omap_host *host)
>>>>> +{
>>>>> +       u32 hctl, capa, value;
>>>>> +
>>>>> +       /* Only MMC1 supports 3.0V */
>>>>> +       if (host->id == OMAP_MMC1_DEVID) {
>>>>> +               hctl = SDVS30;
>>>> Shouldn't it be remembering what voltage it was using,
>>>> and then restore that, instead of always making MMC1
>>>> restart at a 3.0V level?  That's pretty awkward to test
>>>> unless you have a 1.8V-capable card in MMC1...
>>> You are somewhat right, thank you.
>>> But remebering what voltage it was using doesn't feasible to me,
>>> because the card can be changed while in 'Sleep' state. I should have
>>> inserted a function that detect the right voltage after intializing. I
>>> will resend the patch later.
>> Doesn't it already do that? Can you explain more?
>>
>> Although I have not tested it, I very much doubt
>> dual-voltage cards work.  That is because VMMC1_185V
>> is zero, which has the side-effect of turning the
>> regulator off (see arch/arm/mach-omap2/mmc-twl4030.c)
> 
> It's also to difficult to test in our H/W since it's configured only
> support 3.0V.
> 
> How about to separate it two phases, first fix the mmc suspend/resume
> works again, and then verify dual voltage if there are these hardware
> 
> How to you think?

Yes certainly.

The original code assumes that 'host' may be NULL in omap_mmc_suspend ()
and omap_mmc_resume(), whereas the patch assumes 'host' is never NULL.
I suggest:

static int omap_mmc_suspend(struct platform_device *pdev, pm_message_t state)
{
	int ret = 0;
	struct mmc_omap_host *host = platform_get_drvdata(pdev);

-	if (host && host->suspended)
+	if (!host || host->suspended)
		return 0;


and

static int omap_mmc_resume(struct platform_device *pdev)
{
	int ret = 0;
	struct mmc_omap_host *host = platform_get_drvdata(pdev);

-	if (host && !host->suspended)
+	if (!host || !host->suspended)
		return 0;


Also the patch does not wait for the bus voltage (SDBP bit) to
initialise.  For the sake of correctness, I would add something
like:

	OMAP_HSMMC_WRITE(host->base, HCTL, value | SDBP);
+	for (i = 0; i < loops_per_jiffy; i++) {
+		if (OMAP_HSMMC_READ(host->base, HCTL) & SDBP)
+			break;
+		cpu_relax();
+	}


Also, you will need the following patch if you actually want the power
to go off.


From: Adrian Hunter <[e-mail]>
Date: Fri, 30 Jan 2009 11:58:13 +0200
Subject: [PATCH] OMAP: HSMMC: do not power up after powering off

The power is configured when probing and when resuming
so the bus voltage does not need changing after power
off.

Signed-off-by: Adrian Hunter <[e-mail]>
---
 drivers/mmc/host/omap_hsmmc.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

Comments

David Brownell Feb. 23, 2009, 6:23 p.m. UTC | #1

On Monday 23 February 2009, Adrian Hunter wrote:
> Also, you will need the following patch if you actually want the power
> to go off.

Current GIT already has a patch supporting power-off for
MMC2; tested on SDP and some custom hardware.  So this
patch won't apply.

Are you sure that's needed for MMC1?  The led showing MMC1
power did go off correctly (when using MMC2 for root), and
the MMC1 regulator entry in sysfs agreed MMC1 was off.  So
I thought this was only an issue for MMC2 (and presumably
MMC3, though I don't have a board using it.)

I agree that code removed by this patch is ugly and worth
removing if it's not actually needed for MMC1.

- Dave


> From: Adrian Hunter <[e-mail]>
> Date: Fri, 30 Jan 2009 11:58:13 +0200
> Subject: [PATCH] OMAP: HSMMC: do not power up after powering off
> 
> The power is configured when probing and when resuming
> so the bus voltage does not need changing after power
> off.
> 
> Signed-off-by: Adrian Hunter <[e-mail]>
> ---
>  drivers/mmc/host/omap_hsmmc.c |   10 ----------
>  1 files changed, 0 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 1e4a2e0..04e5a0c 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -859,16 +859,6 @@ static void omap_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>         switch (ios->power_mode) {
>         case MMC_POWER_OFF:
>                 mmc_slot(host).set_power(host->dev, host->slot_id, 0, 0);
> -               /*
> -                * Reset bus voltage to 3V if it got set to 1.8V earlier.
> -                * REVISIT: If we are able to detect cards after unplugging
> -                * a 1.8V card, this code should not be needed.
> -                */
> -               if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
> -                       int vdd = fls(host->mmc->ocr_avail) - 1;
> -                       if (omap_mmc_switch_opcond(host, vdd) != 0)
> -                               host->mmc->ios.vdd = vdd;
> -               }
>                 break;
>         case MMC_POWER_UP:
>                 mmc_slot(host).set_power(host->dev, host->slot_id, 1, ios->vdd);



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 1e4a2e0..04e5a0c 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -859,16 +859,6 @@  static void omap_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	switch (ios->power_mode) {
 	case MMC_POWER_OFF:
 		mmc_slot(host).set_power(host->dev, host->slot_id, 0, 0);
-		/*
-		 * Reset bus voltage to 3V if it got set to 1.8V earlier.
-		 * REVISIT: If we are able to detect cards after unplugging
-		 * a 1.8V card, this code should not be needed.
-		 */
-		if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
-			int vdd = fls(host->mmc->ocr_avail) - 1;
-			if (omap_mmc_switch_opcond(host, vdd) != 0)
-				host->mmc->ios.vdd = vdd;
-		}
 		break;
 	case MMC_POWER_UP:
 		mmc_slot(host).set_power(host->dev, host->slot_id, 1, ios->vdd);