[PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Kim Kyuwon]

Most registers lose its state when the processor wakes up from sleep state.
Thus registers should be initialized, when the processor wakes up. However the
current hsmmc 'resume' function doesn't consider this issue and finally makes
deadlock. So this patch fixes this problem.

Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
---
 drivers/mmc/host/omap_hsmmc.c |  155 +++++++++++++++++++++--------------------
 1 files changed, 79 insertions(+), 76 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 56363c5..5a73fa6 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -55,6 +55,7 @@
 #define VS30			(1 << 25)
 #define SDVS18			(0x5 << 9)
 #define SDVS30			(0x6 << 9)
+#define SDVS_MASK		0x00000E00
 #define SDVSCLR			0xFFFFF1FF
 #define SDVSDET			0x00000400
 #define AUTOIDLE		0x1
@@ -861,6 +862,34 @@ static int omap_hsmmc_get_ro(struct mmc_host *mmc)
 	return pdata->slots[0].get_ro(host->dev, 0);
 }

+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;
+		capa = VS30 | VS18;
+	} else {
+		hctl = SDVS18;
+		capa = VS18;
+	}
+
+	value = OMAP_HSMMC_READ(host->base, HCTL) & ~SDVS_MASK;
+	OMAP_HSMMC_WRITE(host->base, HCTL, value | hctl);
+
+	value = OMAP_HSMMC_READ(host->base, CAPA);
+	OMAP_HSMMC_WRITE(host->base, CAPA, value | capa);
+
+	/* Set the controller to AUTO IDLE mode */
+	value = OMAP_HSMMC_READ(host->base, SYSCONFIG);
+	OMAP_HSMMC_WRITE(host->base, SYSCONFIG, value | AUTOIDLE);
+
+	/* Set SD bus power bit */
+	value = OMAP_HSMMC_READ(host->base, HCTL);
+	OMAP_HSMMC_WRITE(host->base, HCTL, value | SDBP);
+}
+
 static struct mmc_host_ops mmc_omap_ops = {
 	.request = omap_mmc_request,
 	.set_ios = omap_mmc_set_ios,
@@ -876,7 +905,6 @@ static int __init omap_mmc_probe(struct
platform_device *pdev)
 	struct mmc_omap_host *host = NULL;
 	struct resource *res;
 	int ret = 0, irq;
-	u32 hctl, capa;

 	if (pdata == NULL) {
 		dev_err(&pdev->dev, "Platform Data is missing\n");
@@ -981,28 +1009,7 @@ static int __init omap_mmc_probe(struct
platform_device *pdev)
 	if (pdata->slots[host->slot_id].wires >= 4)
 		mmc->caps |= MMC_CAP_4_BIT_DATA;

-	/* Only MMC1 supports 3.0V */
-	if (host->id == OMAP_MMC1_DEVID) {
-		hctl = SDVS30;
-		capa = VS30 | VS18;
-	} else {
-		hctl = SDVS18;
-		capa = VS18;
-	}
-
-	OMAP_HSMMC_WRITE(host->base, HCTL,
-			OMAP_HSMMC_READ(host->base, HCTL) | hctl);
-
-	OMAP_HSMMC_WRITE(host->base, CAPA,
-			OMAP_HSMMC_READ(host->base, CAPA) | capa);
-
-	/* Set the controller to AUTO IDLE mode */
-	OMAP_HSMMC_WRITE(host->base, SYSCONFIG,
-			OMAP_HSMMC_READ(host->base, SYSCONFIG) | AUTOIDLE);
-
-	/* Set SD bus power bit */
-	OMAP_HSMMC_WRITE(host->base, HCTL,
-			OMAP_HSMMC_READ(host->base, HCTL) | SDBP);
+	omap_hsmmc_init(host);

 	/* Request IRQ for MMC operations */
 	ret = request_irq(host->irq, mmc_omap_irq, IRQF_DISABLED,
@@ -1127,41 +1134,38 @@ static int omap_mmc_suspend(struct
platform_device *pdev, pm_message_t state)
 	if (host && host->suspended)
 		return 0;

-	if (host) {
-		ret = mmc_suspend_host(host->mmc, state);
-		if (ret == 0) {
-			host->suspended = 1;
-
-			OMAP_HSMMC_WRITE(host->base, ISE, 0);
-			OMAP_HSMMC_WRITE(host->base, IE, 0);
+	ret = mmc_suspend_host(host->mmc, state);
+	if (ret == 0) {
+		host->suspended = 1;

-			if (host->pdata->suspend) {
-				ret = host->pdata->suspend(&pdev->dev,
-								host->slot_id);
-				if (ret)
-					dev_dbg(mmc_dev(host->mmc),
-						"Unable to handle MMC board"
-						" level suspend\n");
-			}
+		OMAP_HSMMC_WRITE(host->base, ISE, 0);
+		OMAP_HSMMC_WRITE(host->base, IE, 0);

-			if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
-				OMAP_HSMMC_WRITE(host->base, HCTL,
-					OMAP_HSMMC_READ(host->base, HCTL)
-					& SDVSCLR);
-				OMAP_HSMMC_WRITE(host->base, HCTL,
-					OMAP_HSMMC_READ(host->base, HCTL)
-					| SDVS30);
-				OMAP_HSMMC_WRITE(host->base, HCTL,
-					OMAP_HSMMC_READ(host->base, HCTL)
-					| SDBP);
-			}
+		if (host->pdata->suspend) {
+			ret = host->pdata->suspend(&pdev->dev, host->slot_id);
+			if (ret)
+				dev_dbg(mmc_dev(host->mmc),
+					"Unable to handle MMC board"
+					" level suspend\n");
+		}

-			clk_disable(host->fclk);
-			clk_disable(host->iclk);
-			clk_disable(host->dbclk);
+		if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
+			OMAP_HSMMC_WRITE(host->base, HCTL,
+				OMAP_HSMMC_READ(host->base, HCTL)
+				& SDVSCLR);
+			OMAP_HSMMC_WRITE(host->base, HCTL,
+				OMAP_HSMMC_READ(host->base, HCTL)
+				| SDVS30);
+			OMAP_HSMMC_WRITE(host->base, HCTL,
+				OMAP_HSMMC_READ(host->base, HCTL)
+				| SDBP);
 		}

+		clk_disable(host->fclk);
+		clk_disable(host->iclk);
+		clk_disable(host->dbclk);
 	}
+
 	return ret;
 }

@@ -1174,36 +1178,35 @@ static int omap_mmc_resume(struct platform_device *pdev)
 	if (host && !host->suspended)
 		return 0;

-	if (host) {
+	ret = clk_enable(host->fclk);
+	if (ret)
+		goto clk_en_err;

-		ret = clk_enable(host->fclk);
-		if (ret)
-			goto clk_en_err;
-
-		ret = clk_enable(host->iclk);
-		if (ret) {
-			clk_disable(host->fclk);
-			clk_put(host->fclk);
-			goto clk_en_err;
-		}
+	ret = clk_enable(host->iclk);
+	if (ret) {
+		clk_disable(host->fclk);
+		clk_put(host->fclk);
+		goto clk_en_err;
+	}

-		if (clk_enable(host->dbclk) != 0)
-			dev_dbg(mmc_dev(host->mmc),
-					"Enabling debounce clk failed\n");
+	if (clk_enable(host->dbclk) != 0)
+		dev_dbg(mmc_dev(host->mmc),
+				"Enabling debounce clk failed\n");

-		if (host->pdata->resume) {
-			ret = host->pdata->resume(&pdev->dev, host->slot_id);
-			if (ret)
-				dev_dbg(mmc_dev(host->mmc),
-					"Unmask interrupt failed\n");
-		}
+	omap_hsmmc_init(host);

-		/* Notify the core to resume the host */
-		ret = mmc_resume_host(host->mmc);
-		if (ret == 0)
-			host->suspended = 0;
+	if (host->pdata->resume) {
+		ret = host->pdata->resume(&pdev->dev, host->slot_id);
+		if (ret)
+			dev_dbg(mmc_dev(host->mmc),
+				"Unmask interrupt failed\n");
 	}

+	/* Notify the core to resume the host */
+	ret = mmc_resume_host(host->mmc);
+	if (ret == 0)
+		host->suspended = 0;
+
 	return ret;

 clk_en_err:
-- 
1.5.2.5


-- 
Kim Kyuwon

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[David Brownell]

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...

Somewhat related:  I think the PBIAS register updates
should be moved out of mach-omap2/mmc-twl4030.c into
this driver.  They're needed no matter what flavor
regulator is used to with MMC1 voltage over 1.8V,
and it's a bit odd to split the state machine for
1.8V -vs- 3.0V I/O voltages the way it's now done.

- Dave

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Kim Kyuwon]

Hi,

On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> 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.

-- 
Kyuwon

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Adrian Hunter]

ext Kim Kyuwon wrote:
> Hi,
> 
> On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> 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)

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Kyungmin Park]

Hi,

On Mon, Feb 23, 2009 at 5:04 PM, Adrian Hunter
<ext-adrian.hunter@nokia.com> wrote:
> ext Kim Kyuwon wrote:
>> Hi,
>>
>> On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> 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?

Thank you,
Kyungmin Park

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Adrian Hunter]

Kyungmin Park wrote:
> Hi,
> 
> On Mon, Feb 23, 2009 at 5:04 PM, Adrian Hunter
> <ext-adrian.hunter@nokia.com> wrote:
>> ext Kim Kyuwon wrote:
>>> Hi,
>>>
>>> On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> 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 <ext-adrian.hunter@nokia.com>
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 <ext-adrian.hunter@nokia.com>
---
 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);
-- 
1.5.4.3

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[David Brownell]

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 <ext-adrian.hunter@nokia.com>
> 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 <ext-adrian.hunter@nokia.com>
> ---
>  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);

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Adrian Hunter]

David Brownell wrote:
> 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.

Sorry, hadn't noticed that.

> 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.)

Yes, if you never switch to 1.8V, then MMC1 is not affected.

However, for dual-voltage cards, if the MMC1 voltage is
switched to 1.8V, then the offending code will leave the
power on, when it should be off i.e. omap_mmc_switch_opcond()
turns the power on, but we are trying to power off.

If you initialise correctly (as Kim Kyuwon's patch does)
then the offending code is not necessary, so it should
just be removed.

I would still like the other two issues I raised considered.
They were:
	1.  'host' can be NULL in omap_mmc_suspend() / omap_mmc_resume()
	2. wait for SDBP bit

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

Here is a patch against current OMAP tree.

From: Adrian Hunter <ext-adrian.hunter@nokia.com>
Date: Tue, 24 Feb 2009 14:48:16 +0200
Subject: [PATCH] OMAP: HSMMC: do not re-power when powering off MMC

Remove code that turns MMC1 power back on after it
has been powered off (when the voltage is 1.8V).

The offending code is not necessary because the
host controller bus voltage is initialized to
3V when probing or resuming.  Note that MMC powers up
with the highest voltage available (see mmc_power_up())
which will be 3V also.

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mmc/host/omap_hsmmc.c |   17 -----------------
 1 files changed, 0 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index c0d5420..43cec98 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -486,9 +486,6 @@ static int omap_mmc_switch_opcond(struct mmc_omap_host *host, int vdd)
 	u32 reg_val = 0;
 	int ret;
 
-	if (host->id != OMAP_MMC1_DEVID)
-		return 0;
-
 	/* Disable the clocks */
 	clk_disable(host->fclk);
 	clk_disable(host->iclk);
@@ -787,20 +784,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 interface voltage to 3V if it's 1.8V now;
-		 * only relevant on MMC-1, the others always use 1.8V.
-		 *
-		 * REVISIT: If we are able to detect cards after unplugging
-		 * a 1.8V card, this code should not be needed.
-		 */
-		if (host->id != OMAP_MMC1_DEVID)
-			break;
-		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);
-- 
1.5.6.3

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[David Brownell]

On Tuesday 24 February 2009, Adrian Hunter wrote:
> > I agree that code removed by this patch is ugly and worth
> > removing if it's not actually needed for MMC1.
> 
> Here is a patch against current OMAP tree.
> 
> From: Adrian Hunter <ext-adrian.hunter@nokia.com>
> Date: Tue, 24 Feb 2009 14:48:16 +0200
> Subject: [PATCH] OMAP: HSMMC: do not re-power when powering off MMC
> 
> Remove code that turns MMC1 power back on after it
> has been powered off (when the voltage is 1.8V).
> 
> The offending code is not necessary because the
> host controller bus voltage is initialized to
> 3V when probing or resuming.  Note that MMC powers up
> with the highest voltage available (see mmc_power_up())
> which will be 3V also.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>

Looks OK to me -- ack.  Safe to merge through the MMC
tree right away, but it'll be a NOP until the glue
actually supports 1.8V correctly for MMC1 ... so IMO
no rush to merge for 2.6.29-final.


> ---
>  drivers/mmc/host/omap_hsmmc.c |   17 -----------------
>  1 files changed, 0 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index c0d5420..43cec98 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -486,9 +486,6 @@ static int omap_mmc_switch_opcond(struct mmc_omap_host *host, int vdd)
>         u32 reg_val = 0;
>         int ret;
>  
> -       if (host->id != OMAP_MMC1_DEVID)
> -               return 0;
> -
>         /* Disable the clocks */
>         clk_disable(host->fclk);
>         clk_disable(host->iclk);
> @@ -787,20 +784,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 interface voltage to 3V if it's 1.8V now;
> -                * only relevant on MMC-1, the others always use 1.8V.
> -                *
> -                * REVISIT: If we are able to detect cards after unplugging
> -                * a 1.8V card, this code should not be needed.
> -                */
> -               if (host->id != OMAP_MMC1_DEVID)
> -                       break;
> -               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);

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [090224 14:14]:
> On Tuesday 24 February 2009, Adrian Hunter wrote:
> > > I agree that code removed by this patch is ugly and worth
> > > removing if it's not actually needed for MMC1.
> > 
> > Here is a patch against current OMAP tree.
> > 
> > From: Adrian Hunter <ext-adrian.hunter@nokia.com>
> > Date: Tue, 24 Feb 2009 14:48:16 +0200
> > Subject: [PATCH] OMAP: HSMMC: do not re-power when powering off MMC
> > 
> > Remove code that turns MMC1 power back on after it
> > has been powered off (when the voltage is 1.8V).
> > 
> > The offending code is not necessary because the
> > host controller bus voltage is initialized to
> > 3V when probing or resuming.  Note that MMC powers up
> > with the highest voltage available (see mmc_power_up())
> > which will be 3V also.
> > 
> > Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> 
> Looks OK to me -- ack.  Safe to merge through the MMC
> tree right away, but it'll be a NOP until the glue
> actually supports 1.8V correctly for MMC1 ... so IMO
> no rush to merge for 2.6.29-final.

Good to hear we get rid of that REVISIT part.. My ack here too.

Acked-by: Tony Lindgren <tony@atomide.com>
 
> > ---
> >  drivers/mmc/host/omap_hsmmc.c |   17 -----------------
> >  1 files changed, 0 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> > index c0d5420..43cec98 100644
> > --- a/drivers/mmc/host/omap_hsmmc.c
> > +++ b/drivers/mmc/host/omap_hsmmc.c
> > @@ -486,9 +486,6 @@ static int omap_mmc_switch_opcond(struct mmc_omap_host *host, int vdd)
> >         u32 reg_val = 0;
> >         int ret;
> >  
> > -       if (host->id != OMAP_MMC1_DEVID)
> > -               return 0;
> > -
> >         /* Disable the clocks */
> >         clk_disable(host->fclk);
> >         clk_disable(host->iclk);
> > @@ -787,20 +784,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 interface voltage to 3V if it's 1.8V now;
> > -                * only relevant on MMC-1, the others always use 1.8V.
> > -                *
> > -                * REVISIT: If we are able to detect cards after unplugging
> > -                * a 1.8V card, this code should not be needed.
> > -                */
> > -               if (host->id != OMAP_MMC1_DEVID)
> > -                       break;
> > -               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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Adrian Hunter]

>From a2ad80abeb9550782d4962472980e47df4055434 Mon Sep 17 00:00:00 2001
From: Kim Kyuwon <chammoru@gmail.com>
Date: Fri, 20 Feb 2009 13:10:08 +0100
Subject: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

Most registers lose its state when the processor wakes up from sleep state.
Thus registers should be initialized, when the processor wakes up. However the
current hsmmc 'resume' function doesn't consider this issue and finally makes
deadlock. So this patch fixes this problem.

Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---




Here is Kim Kyuwon's patch without the 'host is never null in suspend / resume'
assumption, and leaving the issue of SDBP for later.






 drivers/mmc/host/omap_hsmmc.c |   55 +++++++++++++++++++++++-----------------
 1 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index a631c81..1047cb5 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -56,6 +56,7 @@
 #define SDVS18			(0x5 << 9)
 #define SDVS30			(0x6 << 9)
 #define SDVS33			(0x7 << 9)
+#define SDVS_MASK		0x00000E00
 #define SDVSCLR			0xFFFFF1FF
 #define SDVSDET			0x00000400
 #define AUTOIDLE		0x1
@@ -891,6 +892,34 @@ static int omap_hsmmc_get_ro(struct mmc_host *mmc)
 	return pdata->slots[0].get_ro(host->dev, 0);
 }
 
+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;
+		capa = VS30 | VS18;
+	} else {
+		hctl = SDVS18;
+		capa = VS18;
+	}
+
+	value = OMAP_HSMMC_READ(host->base, HCTL) & ~SDVS_MASK;
+	OMAP_HSMMC_WRITE(host->base, HCTL, value | hctl);
+
+	value = OMAP_HSMMC_READ(host->base, CAPA);
+	OMAP_HSMMC_WRITE(host->base, CAPA, value | capa);
+
+	/* Set the controller to AUTO IDLE mode */
+	value = OMAP_HSMMC_READ(host->base, SYSCONFIG);
+	OMAP_HSMMC_WRITE(host->base, SYSCONFIG, value | AUTOIDLE);
+
+	/* Set SD bus power bit */
+	value = OMAP_HSMMC_READ(host->base, HCTL);
+	OMAP_HSMMC_WRITE(host->base, HCTL, value | SDBP);
+}
+
 static struct mmc_host_ops mmc_omap_ops = {
 	.request = omap_mmc_request,
 	.set_ios = omap_mmc_set_ios,
@@ -906,7 +935,6 @@ static int __init omap_mmc_probe(struct platform_device *pdev)
 	struct mmc_omap_host *host = NULL;
 	struct resource *res;
 	int ret = 0, irq;
-	u32 hctl, capa;
 
 	if (pdata == NULL) {
 		dev_err(&pdev->dev, "Platform Data is missing\n");
@@ -1011,28 +1039,7 @@ static int __init omap_mmc_probe(struct platform_device *pdev)
 	if (pdata->slots[host->slot_id].wires >= 4)
 		mmc->caps |= MMC_CAP_4_BIT_DATA;
 
-	/* Only MMC1 supports 3.0V */
-	if (host->id == OMAP_MMC1_DEVID) {
-		hctl = SDVS30;
-		capa = VS30 | VS18;
-	} else {
-		hctl = SDVS18;
-		capa = VS18;
-	}
-
-	OMAP_HSMMC_WRITE(host->base, HCTL,
-			OMAP_HSMMC_READ(host->base, HCTL) | hctl);
-
-	OMAP_HSMMC_WRITE(host->base, CAPA,
-			OMAP_HSMMC_READ(host->base, CAPA) | capa);
-
-	/* Set the controller to AUTO IDLE mode */
-	OMAP_HSMMC_WRITE(host->base, SYSCONFIG,
-			OMAP_HSMMC_READ(host->base, SYSCONFIG) | AUTOIDLE);
-
-	/* Set SD bus power bit */
-	OMAP_HSMMC_WRITE(host->base, HCTL,
-			OMAP_HSMMC_READ(host->base, HCTL) | SDBP);
+	omap_hsmmc_init(host);
 
 	/* Request IRQ for MMC operations */
 	ret = request_irq(host->irq, mmc_omap_irq, IRQF_DISABLED,
@@ -1222,6 +1229,8 @@ static int omap_mmc_resume(struct platform_device *pdev)
 			dev_dbg(mmc_dev(host->mmc),
 					"Enabling debounce clk failed\n");
 
+		omap_hsmmc_init(host);
+
 		if (host->pdata->resume) {
 			ret = host->pdata->resume(&pdev->dev, host->slot_id);
 			if (ret)
-- 
1.5.6.3

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Tony Lindgren]

* Adrian Hunter <adrian.hunter@nokia.com> [090302 04:27]:
> From a2ad80abeb9550782d4962472980e47df4055434 Mon Sep 17 00:00:00 2001
> From: Kim Kyuwon <chammoru@gmail.com>
> Date: Fri, 20 Feb 2009 13:10:08 +0100
> Subject: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming
>
> Most registers lose its state when the processor wakes up from sleep state.
> Thus registers should be initialized, when the processor wakes up. However the
> current hsmmc 'resume' function doesn't consider this issue and finally makes
> deadlock. So this patch fixes this problem.
>
> Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>

Sounds like there's no rush to get this into mainline until during the
next merge window as we're still missing most of the 24xx and 34xx PM code.

Acked-by: Tony Lindgren <tony@atomide.com>


> ---
>
>
>
>
> Here is Kim Kyuwon's patch without the 'host is never null in suspend / resume'
> assumption, and leaving the issue of SDBP for later.
>
>
>
>
>
>
> drivers/mmc/host/omap_hsmmc.c |   55 +++++++++++++++++++++++-----------------
> 1 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index a631c81..1047cb5 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -56,6 +56,7 @@
> #define SDVS18			(0x5 << 9)
> #define SDVS30			(0x6 << 9)
> #define SDVS33			(0x7 << 9)
> +#define SDVS_MASK		0x00000E00
> #define SDVSCLR			0xFFFFF1FF
> #define SDVSDET			0x00000400
> #define AUTOIDLE		0x1
> @@ -891,6 +892,34 @@ static int omap_hsmmc_get_ro(struct mmc_host *mmc)
> 	return pdata->slots[0].get_ro(host->dev, 0);
> }
>
> +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;
> +		capa = VS30 | VS18;
> +	} else {
> +		hctl = SDVS18;
> +		capa = VS18;
> +	}
> +
> +	value = OMAP_HSMMC_READ(host->base, HCTL) & ~SDVS_MASK;
> +	OMAP_HSMMC_WRITE(host->base, HCTL, value | hctl);
> +
> +	value = OMAP_HSMMC_READ(host->base, CAPA);
> +	OMAP_HSMMC_WRITE(host->base, CAPA, value | capa);
> +
> +	/* Set the controller to AUTO IDLE mode */
> +	value = OMAP_HSMMC_READ(host->base, SYSCONFIG);
> +	OMAP_HSMMC_WRITE(host->base, SYSCONFIG, value | AUTOIDLE);
> +
> +	/* Set SD bus power bit */
> +	value = OMAP_HSMMC_READ(host->base, HCTL);
> +	OMAP_HSMMC_WRITE(host->base, HCTL, value | SDBP);
> +}
> +
> static struct mmc_host_ops mmc_omap_ops = {
> 	.request = omap_mmc_request,
> 	.set_ios = omap_mmc_set_ios,
> @@ -906,7 +935,6 @@ static int __init omap_mmc_probe(struct platform_device *pdev)
> 	struct mmc_omap_host *host = NULL;
> 	struct resource *res;
> 	int ret = 0, irq;
> -	u32 hctl, capa;
>
> 	if (pdata == NULL) {
> 		dev_err(&pdev->dev, "Platform Data is missing\n");
> @@ -1011,28 +1039,7 @@ static int __init omap_mmc_probe(struct platform_device *pdev)
> 	if (pdata->slots[host->slot_id].wires >= 4)
> 		mmc->caps |= MMC_CAP_4_BIT_DATA;
>
> -	/* Only MMC1 supports 3.0V */
> -	if (host->id == OMAP_MMC1_DEVID) {
> -		hctl = SDVS30;
> -		capa = VS30 | VS18;
> -	} else {
> -		hctl = SDVS18;
> -		capa = VS18;
> -	}
> -
> -	OMAP_HSMMC_WRITE(host->base, HCTL,
> -			OMAP_HSMMC_READ(host->base, HCTL) | hctl);
> -
> -	OMAP_HSMMC_WRITE(host->base, CAPA,
> -			OMAP_HSMMC_READ(host->base, CAPA) | capa);
> -
> -	/* Set the controller to AUTO IDLE mode */
> -	OMAP_HSMMC_WRITE(host->base, SYSCONFIG,
> -			OMAP_HSMMC_READ(host->base, SYSCONFIG) | AUTOIDLE);
> -
> -	/* Set SD bus power bit */
> -	OMAP_HSMMC_WRITE(host->base, HCTL,
> -			OMAP_HSMMC_READ(host->base, HCTL) | SDBP);
> +	omap_hsmmc_init(host);
>
> 	/* Request IRQ for MMC operations */
> 	ret = request_irq(host->irq, mmc_omap_irq, IRQF_DISABLED,
> @@ -1222,6 +1229,8 @@ static int omap_mmc_resume(struct platform_device *pdev)
> 			dev_dbg(mmc_dev(host->mmc),
> 					"Enabling debounce clk failed\n");
>
> +		omap_hsmmc_init(host);
> +
> 		if (host->pdata->resume) {
> 			ret = host->pdata->resume(&pdev->dev, host->slot_id);
> 			if (ret)
> -- 
> 1.5.6.3

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Pierre Ossman]

On Mon, 2 Mar 2009 08:44:14 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Adrian Hunter <adrian.hunter@nokia.com> [090302 04:27]:
> > From a2ad80abeb9550782d4962472980e47df4055434 Mon Sep 17 00:00:00 2001
> > From: Kim Kyuwon <chammoru@gmail.com>
> > Date: Fri, 20 Feb 2009 13:10:08 +0100
> > Subject: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming
> >
> > Most registers lose its state when the processor wakes up from sleep state.
> > Thus registers should be initialized, when the processor wakes up. However the
> > current hsmmc 'resume' function doesn't consider this issue and finally makes
> > deadlock. So this patch fixes this problem.
> >
> > Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
> > Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> 
> Sounds like there's no rush to get this into mainline until during the
> next merge window as we're still missing most of the 24xx and 34xx PM code.
> 
> Acked-by: Tony Lindgren <tony@atomide.com>
> 
> 

Queued up both patches for the next merge window.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[David Brownell]

On Tuesday 24 February 2009, Adrian Hunter wrote:
> I would still like the other two issues I raised considered.
> They were:
>         1.  'host' can be NULL in omap_mmc_suspend() / omap_mmc_resume()
>         2. wait for SDBP bit

Someone from Nokia was going to be shepherding HSMMC patches,
as I recall... that person should deal with these issues.  :)

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[David Brownell]

On Monday 23 February 2009, Kyungmin Park wrote:
> > 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)

Right, looks like a longstanding bug in that glue.
So IMO, no rush to fix for 2.6.29-rc ...


> It's also to difficult to test in our H/W since it's configured only
> support 3.0V.

Easily tested with a Beagle or SDP though, if you
have a 1.8V or dual-voltage card (maybe RS-MMC, as
for an N770 tablet).


> 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

Well, two patches for sure; I don't know that the order
will matter.  I just sent a patch fixing that code in
the current OMAP git tree, which has some fixes that are
scheduled to merge for 2.6.30-rc.

- Dave

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[David Brownell]

On Monday 23 February 2009, Adrian Hunter wrote:
> 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)

And a second reason to know they don't quite work ... in
the file drivers/mmc/host/omap_hsmmc.c, omap_mmc_set_ios()
sets the voltage for MMC_POWER_OFF (0) or MMC_POWER_UP (1_,
which gives the initial setting -- e.g. 3.15 Volts, so it
can enumerate at the high range.

But after enumerating the card at that voltage, checking
the OCR values, and concluding that the slot and card can
both run at 1.85V ... the MMC_POWER_ON (2) code is used.
But the driver completely ignores it ... the low voltage
(more power-efficient!) voltage range never kicks in.

It'd be nice to have a nice unambiguous set_voltage()
request from the MMC core.  The set_ios() thing has
always been confusing.

- Dave

Re: [PATCH] OMAP: HSMMC: Initialize hsmmc controller registers when resuming

[Pierre Ossman]

On Tue, 10 Mar 2009 19:33:50 -0800
David Brownell <david-b@pacbell.net> wrote:

> 
> It'd be nice to have a nice unambiguous set_voltage()
> request from the MMC core.  The set_ios() thing has
> always been confusing.
> 

set_ios() should be taken out back. But someone has to have the time to
reimplement things. :/

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.