| Message ID | 20110605123852.BC6F39D401C@zog.reactivated.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Sun, Jun 5, 2011 at 3:38 PM, Daniel Drake <dsd@laptop.org> wrote: > @@ -706,10 +706,25 @@ static int mmc_sdio_power_restore(struct mmc_host *host) > BUG_ON(!host->card); > > mmc_claim_host(host); > + > + /* > + * Reset the card by performing the same steps that are taken by > + * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe > + */ > + sdio_reset(host); Sending a reset is necessary only if we are re-initing a powered on card, but in this case, we know that we just powered the card up, so this is not needed. + ret = mmc_send_io_op_cond(host, 0, NULL); + if (ret) + goto out; Can you please add this under a card quirk ? The spec does not require this for embedded sdio cards, and we would like to skip this for wl12xx. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 June 2011 14:48, Ohad Ben-Cohen <ohad@wizery.com> wrote: >> + >> + /* >> + * Reset the card by performing the same steps that are taken by >> + * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe >> + */ >> + sdio_reset(host); > > Sending a reset is necessary only if we are re-initing a powered on > card, but in this case, we know that we just powered the card up, so > this is not needed. Don't ask me for an explanation, but this is required here, for the following scenario: 1. Power up and probe 2. Power down via runtime pm 3. Power up step 3 needs the reset (even though it wasnt needed in step 1), otherwise mmc_send_io_op_cond() fails with a timeout. It is also needed for: 1. Power up 2. Suspend, libertas handler returns -ENOSYS so card gets powered down 3. System resume 4. Reprobe needs to power up card Step 4 fails without this reset. > + ret = mmc_send_io_op_cond(host, 0, NULL); > + if (ret) > + goto out; > > Can you please add this under a card quirk ? > > The spec does not require this for embedded sdio cards, and we would > like to skip this for wl12xx. I could do this if it is required for merge, but I don't agree with it. Are you suggesting that a quirk is used to enable this reset, or to disable it? It seems overkill for something that is going to be trivial and harmless to your setup. Also, the most frustrating thing when working on this issue is that the codepaths for powering up a SD card between the following 3 situations are significantly different: 1. Power up and probe during boot 2. Resume from suspend 3. Power up after runtime suspend It feels like there should be a lot more in common here. My patch is adding some coherence to the process (and there is still plenty of room for improvement, but adding quirks won't help...). Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 7, 2011 at 7:41 PM, Daniel Drake <dsd@laptop.org> wrote: > On 5 June 2011 14:48, Ohad Ben-Cohen <ohad@wizery.com> wrote: >> Sending a reset is necessary only if we are re-initing a powered on >> card, but in this case, we know that we just powered the card up, so >> this is not needed. > > Don't ask me for an explanation, but this is required here Uhm, strange. Maybe this has something to do with the sd8686's reset line which you are not toggling ? Btw, the previous patch which you sent didn't have this reset command, and the patch did work for you. What has changed in this respect ? > Are you suggesting that a quirk is used to enable this reset, or > to disable it? To disable the CMD5 arg=0. it's not mandatory by the spec, and some cards don't need it. It's not harmful either way, but it's just cleaner for those cards (and it's really just adding 1 'if' statement..). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 7, 2011 at 7:41 PM, Daniel Drake <dsd@laptop.org> wrote: > Also, the most frustrating thing when working on this issue is that > the codepaths for powering up a SD card between the following 3 > situations are significantly different: > 1. Power up and probe during boot > 2. Resume from suspend > 3. Power up after runtime suspend > > It feels like there should be a lot more in common here. Definitely agree. I also suggested this in my first reply to you on our other thread: http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08193.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 8, 2011 at 12:20 PM, Daniel Drake <dsd@laptop.org> wrote: > In the first patch I sent I had only tested basic power-up after > probe, and found it to be working. Then I tested the other scenarios > listed in my last mail - basically powering it off and on *again* - > and found them to be broken, but fixed by further mirroring what > existing non-rpm code would do in order to power up a SD card. Sounds like you didn't power the card off then in the driver after probe ? Can you show the diff with which you did this experiment ? There is no difference between powering the card up in sdio_bus_probe to powering it up later in the driver. If the card was indeed powered off beforehand, then the latter should work too. Your reset command strongly suggests that wasn't the case. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index 8af3330..9170ea2 100644 --- a/drivers/mmc/core/sdio.c +++ b/drivers/mmc/core/sdio.c @@ -706,10 +706,25 @@ static int mmc_sdio_power_restore(struct mmc_host *host) BUG_ON(!host->card); mmc_claim_host(host); + + /* + * Reset the card by performing the same steps that are taken by + * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe + */ + sdio_reset(host); + mmc_go_idle(host); + mmc_send_if_cond(host, host->ocr_avail); + + ret = mmc_send_io_op_cond(host, 0, NULL); + if (ret) + 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;
mmc_sdio_power_restore() skips some steps that are performed in other power-related codepaths which are necessary to fully reset the card. Without this, the card can't be powered up and probe fails. All these steps are needed, to satisfy the cases of both normal runtime PM and also suspend/resume situations. Tested on sd8686 libertas wifi on XO-1.5. Signed-off-by: Daniel Drake <dsd@laptop.org> --- drivers/mmc/core/sdio.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)