MMC runtime PM patches break libertas probe

Commit Message

Daniel Drake May 29, 2011, 4:21 p.m. UTC

Hi Ohad,

On 31 October 2010 19:06, Ohad Ben-Cohen <[e-mail]> wrote:
> OK, as expected.
>
> So to summarize:
> 1. Card is powered up at boot, and successfully initialized
> 2. After mmc + sdio devices are added to the device tree, power is
> (seemingly) taken down by runtime PM
> 3. When the driver is loaded, card is powered up again, but doesn't
> respond to CMD3
>
> The only explanation I can think of why the card doesn't respond to
> CMD3, after its power is brought up again, is that we didn't have a
> full reset here (i.e. mmc_power_off() didn't completely power off
> everything).

I have investigated this again, as we'd like runtime PM to work.

It's certainly possible that there's something weird about the
hardware in question, but we *are* able to successfully power down and
up the card with a hacky rfkill driver that calls mmc_stop_host /
mmc_start_host. So I went around finding out what the difference was
between these functions and the runtime PM implementation.

Through this comparison I think mmc_power_save_host() does almost
exactly the same as mmc_stop_host() (good), but
mmc_power_restore_host() lacks some steps which would otherwise be
taken by mmc_start_host(). These are:

in mmc_rescan_try_freq():

	/*
	 * sdio_reset sends CMD52 to reset card.  Since we do not know
	 * if the card is being re-initialized, just send it.  CMD52
	 * should be ignored by SD/eMMC cards.
	 */
	sdio_reset(host);
	mmc_go_idle(host);

	mmc_send_if_cond(host, host->ocr_avail);


In mmc_attach_sdio():

	err = mmc_send_io_op_cond(host, 0, &ocr);
	if (err)
		return err;

	mmc_attach_bus(host, &mmc_sdio_ops);
	if (host->ocr_avail_sdio)
		host->ocr_avail = host->ocr_avail_sdio;

	/*
	 * Sanity check the voltages that the card claims to
	 * support.
	 */
	if (ocr & 0x7F) {
		printk(KERN_WARNING "%s: card claims to support voltages "
		       "below the defined range. These will be ignored.\n",
		       mmc_hostname(host));
		ocr &= ~0x7F;
	}

	host->ocr = mmc_select_voltage(host, ocr);

	/*
	 * Can we support the voltage(s) of the card(s)?
	 */
	if (!host->ocr) {
		err = -EINVAL;
		goto err;
	}


Should anything in those code snippets be running during runtime PM resume?

I went ahead and ran the obvious test by putting those bits of code in
the runtime PM resume path... the first snippet doesn't seem to
improve or hurt anything, but the second snippet fixes the problem. At
least it means I can boot, the card gets powered down during boot,
then I load the module and it powers up and initialises correctly.
Patch attached for clarity.

Any thoughts?

Thanks,
Daniel

Comments

Bing Zhao June 2, 2011, 8:39 a.m. UTC | #1

Hi Ohad,

> If we power down the sd8686, and power it up again, without toggling
> the reset pin at all (e.g. keep it always high), will the card work ?

Yes. The card should work without toggling the reset pin.

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

Ohad Ben Cohen June 2, 2011, 6:25 p.m. UTC | #2

On Thu, Jun 2, 2011 at 11:39 AM, Bing Zhao <[e-mail]> wrote:
>> If we power down the sd8686, and power it up again, without toggling
>> the reset pin at all (e.g. keep it always high), will the card work ?
>
> Yes. The card should work without toggling the reset pin.

Thanks.

Just for closure-sake, can you please confirm that the sd8686 requires
sending a CMD5 arg=0 as part of the initialization sequence after
powering it on ?

(we'd probably add it anyway, but it'd be nice to get a confirmation
about this from you guys)

Btw, it will be nice to allow cards to avoid sending this if not
required; a simple card quirk will do. wl12xx will definitely use such
a quirk.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bing Zhao June 3, 2011, 10:28 p.m. UTC | #3

> On Thu, Jun 2, 2011 at 11:39 AM, Bing Zhao <[e-mail]> wrote:
>>> If we power down the sd8686, and power it up again, without toggling
>>> the reset pin at all (e.g. keep it always high), will the card work ?
>>
>> Yes. The card should work without toggling the reset pin.
> 
> Thanks.
> 
> Just for closure-sake, can you please confirm that the sd8686 requires
> sending a CMD5 arg=0 as part of the initialization sequence after
> powering it on ?

"CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).

Regards,
Bing

> (we'd probably add it anyway, but it'd be nice to get a confirmation
> about this from you guys)
> 
> Btw, it will be nice to allow cards to avoid sending this if not
> required; a simple card quirk will do. wl12xx will definitely use such
> a quirk.--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ohad Ben Cohen June 3, 2011, 10:52 p.m. UTC | #4

(cc'ing Arnd Hannermann)

On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <[e-mail]> wrote:
> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).

Great, thanks for confirming this !

Arnd, you have reported SDIO runtime pm issues with the b43 in the
past. If you're still interested and have some free cycles, you might
want to check out Daniel's patch and see if that fixes the issues you
had too. With Daniel's patch we're now following the spec more
strictly, and the change is particularly relevant for removable SDIO
cards like the one you were using.

Daniel, please go ahead and submit your patch when you're ready.

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

Arnd Hannemann June 7, 2011, 2:34 p.m. UTC | #5

Hi Ohad,

Am 04.06.2011 00:52, schrieb Ohad Ben-Cohen:
> (cc'ing Arnd Hannermann)
> 
> On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <[e-mail]> wrote:
>> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
>> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).
> 
> Great, thanks for confirming this !
> 
> Arnd, you have reported SDIO runtime pm issues with the b43 in the
> past. If you're still interested and have some free cycles, you might
> want to check out Daniel's patch and see if that fixes the issues you
> had too. With Daniel's patch we're now following the spec more
> strictly, and the change is particularly relevant for removable SDIO
> cards like the one you were using.

Sorry, I don't have the hardware anymore so I'm not able to check this.

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

Ohad Ben Cohen June 7, 2011, 2:45 p.m. UTC | #6

On Tue, Jun 7, 2011 at 5:34 PM, Arnd Hannemann <[e-mail]> wrote:
> Sorry, I don't have the hardware anymore so I'm not able to check this.

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

Ohad Ben Cohen June 8, 2011, 2:34 p.m. UTC | #7

Hi Bing,

On Sat, Jun 4, 2011 at 1:52 AM, Ohad Ben-Cohen <[e-mail]> wrote:
> On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <[e-mail]> wrote:
>> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
>> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).
>
> Great, thanks for confirming this !

I have another question please.

Does the sd8686 require an SDIO I/O reset (CMD52 setting bit 3 of
address 0x6 of the CCCR) to it after powering it on ?

Daniel (cc'ed) is trying to power it off and back on, and it does seem
to work in the first time, without sending a reset. In the second
time, though, the card doesn't answer CMD5 anymore, unless Daniel is
sending it an SDIO I/O reset. I was wondering whether this is an
sd8686 requirement, or whether we have some other issue at hand.

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

Zhangfei Gao June 10, 2011, 2:02 a.m. UTC | #8

On Wed, Jun 8, 2011 at 10:34 PM, Ohad Ben-Cohen <[e-mail]> wrote:
> Hi Bing,
>
> On Sat, Jun 4, 2011 at 1:52 AM, Ohad Ben-Cohen <[e-mail]> wrote:
>> On Sat, Jun 4, 2011 at 1:28 AM, Bing Zhao <[e-mail]> wrote:
>>> "CMD5 Arg=0" refers to the very first CMD5 sent from host during initialization sequence.
>>> This is required because our state machine always expects two CMD5 from host (5, 5, 3, 7, ...).
>>
>> Great, thanks for confirming this !
>
> I have another question please.
>
> Does the sd8686 require an SDIO I/O reset (CMD52 setting bit 3 of
> address 0x6 of the CCCR) to it after powering it on ?
>
> Daniel (cc'ed) is trying to power it off and back on, and it does seem
> to work in the first time, without sending a reset. In the second
> time, though, the card doesn't answer CMD5 anymore, unless Daniel is
> sending it an SDIO I/O reset. I was wondering whether this is an
> sd8686 requirement, or whether we have some other issue at hand.

Hi, Ohad

Here is answer got from the sd8686 maintainer.

For 8686, the SDIO state machine can only handle init sequence (CMD5,
5, 3, 7) from host once. If host sends another init sequence, it will
not be able to handle CMD5 and causes the SDIO block to hang. Chips
that are newer than 8686 will be able to handle multiple init sequence
from host.

So yes, for 8686, an IO reset is needed before host can send a new set
of init sequence.


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

Ohad Ben Cohen June 10, 2011, 4:28 a.m. UTC | #9

Hi Zhangfei,

On Fri, Jun 10, 2011 at 5:02 AM, zhangfei gao <[e-mail]> wrote:
> Here is answer got from the sd8686 maintainer.
>
> For 8686, the SDIO state machine can only handle init sequence (CMD5,
> 5, 3, 7) from host once. If host sends another init sequence, it will
> not be able to handle CMD5 and causes the SDIO block to hang. Chips
> that are newer than 8686 will be able to handle multiple init sequence
> from host.

Thanks for the reply !

> So yes, for 8686, an IO reset is needed before host can send a new set
> of init sequence.

But if we're powering down and up the device first, then the init
sequence is considered the first one, and then we don't need an IO
reset, right ? That was what we wondered about.

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

Zhangfei Gao June 11, 2011, 2:33 a.m. UTC | #10

On Fri, Jun 10, 2011 at 12:28 PM, Ohad Ben-Cohen <[e-mail]> wrote:
> Hi Zhangfei,
>
> On Fri, Jun 10, 2011 at 5:02 AM, zhangfei gao <[e-mail]> wrote:
>> Here is answer got from the sd8686 maintainer.
>>
>> For 8686, the SDIO state machine can only handle init sequence (CMD5,
>> 5, 3, 7) from host once. If host sends another init sequence, it will
>> not be able to handle CMD5 and causes the SDIO block to hang. Chips
>> that are newer than 8686 will be able to handle multiple init sequence
>> from host.
>
> Thanks for the reply !
>
>> So yes, for 8686, an IO reset is needed before host can send a new set
>> of init sequence.
>
> But if we're powering down and up the device first, then the init
> sequence is considered the first one, and then we don't need an IO
> reset, right ? That was what we wondered about.

Hi Ohad,

If you power down and up the device, then IO reset is not needed and
8686 can process host init sequence correctly.

CC Benson.

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

Ohad Ben Cohen June 11, 2011, 9:03 a.m. UTC | #11

On Sat, Jun 11, 2011 at 5:33 AM, zhangfei gao <[e-mail]> wrote:
> If you power down and up the device, then IO reset is not needed and
> 8686 can process host init sequence correctly.

Thanks, that's what I thought.

Daniel, this ultimately means that whenever sending a reset is
required to re-init the 8686, we can surely say that the chip wasn't
really powered off beforehand, and that something went wrong in the
software, leading us to think the chip is powered off when it is
really not.

But I think we also demonstrated this with the simple
insmod/rmmod/insmod scenario, where every insmod successfully
re-initialized the chip without sending an sdio reset.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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/core/sdio.c b/drivers/mmc/core/sdio.c
index 4d0c15b..3b06379 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -691,15 +691,47 @@  static int mmc_sdio_resume(struct mmc_host *host)
 static int mmc_sdio_power_restore(struct mmc_host *host)
 {
 	int ret;
+	u32 ocr;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
+
+	ret = mmc_send_io_op_cond(host, 0, &ocr);
+	if (ret)
+		goto out;
+
+	if (host->ocr_avail_sdio)
+		host->ocr_avail = host->ocr_avail_sdio;
+
+	/*
+	 * Sanity check the voltages that the card claims to
+	 * support.
+	 */
+	if (ocr & 0x7F) {
+		printk(KERN_WARNING "%s: card claims to support voltages "
+		       "below the defined range. These will be ignored.\n",
+		       mmc_hostname(host));
+		ocr &= ~0x7F;
+	}
+
+	host->ocr = mmc_select_voltage(host, ocr);
+
+	/*
+	 * Can we support the voltage(s) of the card(s)?
+	 */
+	if (!host->ocr) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
+
+out:
 	mmc_release_host(host);
 
 	return ret;