Re: MMC runtime PM patches break libertas probe

[Daniel Drake <dsd@laptop.org>]

[-- Attachment #1: Type: text/plain, Size: 2601 bytes --]

Hi Ohad,

On 31 October 2010 19:06, Ohad Ben-Cohen <ohad@wizery.com> 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

[-- Attachment #2: fix-sdio-power-restore.txt --]
[-- Type: text/plain, Size: 1166 bytes --]

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;