| Message ID | 1429531796-21987-9-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: > Currently "mmc sleep" is used before power off and > is not paired with waking up. Nevertheless hold > re-tuning. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/mmc.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index f36c76f..daf9954 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -21,6 +21,7 @@ > #include <linux/mmc/mmc.h> > > #include "core.h" > +#include "host.h" > #include "bus.h" > #include "mmc_ops.h" > #include "sd_ops.h" > @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) > return (card && card->ext_csd.rev >= 3); > } > > +/* If necessary, callers must hold re-tuning */ > static int mmc_sleep(struct mmc_host *host) > { > struct mmc_command cmd = {0}; > @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > int err = 0; > unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : > EXT_CSD_POWER_OFF_LONG; > + bool retune_release = false; > > BUG_ON(!host); > BUG_ON(!host->card); > @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > goto out; > > if (mmc_can_poweroff_notify(host->card) && > - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) > + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { > err = mmc_poweroff_notify(host->card, notify_type); > - else if (mmc_can_sleep(host->card)) > + } else if (mmc_can_sleep(host->card)) { > + mmc_retune_hold(host); > err = mmc_sleep(host); > - else if (!mmc_host_is_spi(host)) > + } else if (!mmc_host_is_spi(host)) { > err = mmc_deselect_cards(host); > + } > > if (!err) { > mmc_power_off(host); > mmc_card_set_suspended(host->card); > } > + > + if (retune_release) > + mmc_retune_release(host); > out: > mmc_release_host(host); > return err; > -- > 1.9.1 > According to our previous discussions I have given this some more thinking. I don't think we can allow to hold/disable re-tune in this path at all. That's because we are claiming the host here and the sleep command might then be the first command we invoke during the system PM sequence. That means sdhci might have flagged need_retune, since it's been runtime PM suspended. And for those scenarios I guess we really need to do a re-tune prior sending the sleep command, right? Earlier I only had the re-tune timer in mind, which is why I was less restrictive and suggesting you to add hold/disable. Sorry about that. Now, with the above in mind I believe you have similar issues with patch5 (mmc: core: Hold re-tuning during switch commands) and patch6 (mmc: core: Hold re-tuning during erase commands). And that's because there are cases when the switch/erase commands are the first commands sent, after the sdhci host has been runtime PM suspended. I guess we need a way to make sure we don't hold re-tune for these cases. An option to deal with that is to use a separate flag set by host drivers, though the mmc_needs_retune() API and let that one override another. Forgive me for pushing you back and forth for how to do this, but it seems like we still have some outstanding issues to resolve. Kind regards Uffe -- 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 21/04/15 12:42, Ulf Hansson wrote: > On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: >> Currently "mmc sleep" is used before power off and >> is not paired with waking up. Nevertheless hold >> re-tuning. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/mmc/core/mmc.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >> index f36c76f..daf9954 100644 >> --- a/drivers/mmc/core/mmc.c >> +++ b/drivers/mmc/core/mmc.c >> @@ -21,6 +21,7 @@ >> #include <linux/mmc/mmc.h> >> >> #include "core.h" >> +#include "host.h" >> #include "bus.h" >> #include "mmc_ops.h" >> #include "sd_ops.h" >> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >> return (card && card->ext_csd.rev >= 3); >> } >> >> +/* If necessary, callers must hold re-tuning */ >> static int mmc_sleep(struct mmc_host *host) >> { >> struct mmc_command cmd = {0}; >> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >> int err = 0; >> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >> EXT_CSD_POWER_OFF_LONG; >> + bool retune_release = false; >> >> BUG_ON(!host); >> BUG_ON(!host->card); >> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >> goto out; >> >> if (mmc_can_poweroff_notify(host->card) && >> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >> err = mmc_poweroff_notify(host->card, notify_type); >> - else if (mmc_can_sleep(host->card)) >> + } else if (mmc_can_sleep(host->card)) { >> + mmc_retune_hold(host); >> err = mmc_sleep(host); >> - else if (!mmc_host_is_spi(host)) >> + } else if (!mmc_host_is_spi(host)) { >> err = mmc_deselect_cards(host); >> + } >> >> if (!err) { >> mmc_power_off(host); >> mmc_card_set_suspended(host->card); >> } >> + >> + if (retune_release) >> + mmc_retune_release(host); >> out: >> mmc_release_host(host); >> return err; >> -- >> 1.9.1 >> > > According to our previous discussions I have given this some more thinking. > > I don't think we can allow to hold/disable re-tune in this path at > all. That's because we are claiming the host here and the sleep > command might then be the first command we invoke during the system PM > sequence. > > That means sdhci might have flagged need_retune, since it's been > runtime PM suspended. And for those scenarios I guess we really need > to do a re-tune prior sending the sleep command, right? Yes, although that is how it works. Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold() but after one of the revisions I found that only one was needed. I stuck with the mmc_retune_hold() name because it doesn't necessarily cause a re-tune, but only if the hold count was zero and a retune is needed. > > Earlier I only had the re-tune timer in mind, which is why I was less > restrictive and suggesting you to add hold/disable. Sorry about that. > > Now, with the above in mind I believe you have similar issues with > patch5 (mmc: core: Hold re-tuning during switch commands) and patch6 > (mmc: core: Hold re-tuning during erase commands). And that's because > there are cases when the switch/erase commands are the first commands > sent, after the sdhci host has been runtime PM suspended. I guess we > need a way to make sure we don't hold re-tune for these cases. > > An option to deal with that is to use a separate flag set by host > drivers, though the mmc_needs_retune() API and let that one override > another. > > Forgive me for pushing you back and forth for how to do this, but it Not a problem. Thanks for persevering. > seems like we still have some outstanding issues to resolve. -- 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 21 April 2015 at 13:00, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 21/04/15 12:42, Ulf Hansson wrote: >> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> Currently "mmc sleep" is used before power off and >>> is not paired with waking up. Nevertheless hold >>> re-tuning. >>> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>> --- >>> drivers/mmc/core/mmc.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index f36c76f..daf9954 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -21,6 +21,7 @@ >>> #include <linux/mmc/mmc.h> >>> >>> #include "core.h" >>> +#include "host.h" >>> #include "bus.h" >>> #include "mmc_ops.h" >>> #include "sd_ops.h" >>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >>> return (card && card->ext_csd.rev >= 3); >>> } >>> >>> +/* If necessary, callers must hold re-tuning */ >>> static int mmc_sleep(struct mmc_host *host) >>> { >>> struct mmc_command cmd = {0}; >>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>> int err = 0; >>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >>> EXT_CSD_POWER_OFF_LONG; >>> + bool retune_release = false; >>> >>> BUG_ON(!host); >>> BUG_ON(!host->card); >>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>> goto out; >>> >>> if (mmc_can_poweroff_notify(host->card) && >>> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >>> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >>> err = mmc_poweroff_notify(host->card, notify_type); >>> - else if (mmc_can_sleep(host->card)) >>> + } else if (mmc_can_sleep(host->card)) { >>> + mmc_retune_hold(host); >>> err = mmc_sleep(host); >>> - else if (!mmc_host_is_spi(host)) >>> + } else if (!mmc_host_is_spi(host)) { >>> err = mmc_deselect_cards(host); >>> + } >>> >>> if (!err) { >>> mmc_power_off(host); >>> mmc_card_set_suspended(host->card); >>> } >>> + >>> + if (retune_release) >>> + mmc_retune_release(host); >>> out: >>> mmc_release_host(host); >>> return err; >>> -- >>> 1.9.1 >>> >> >> According to our previous discussions I have given this some more thinking. >> >> I don't think we can allow to hold/disable re-tune in this path at >> all. That's because we are claiming the host here and the sleep >> command might then be the first command we invoke during the system PM >> sequence. >> >> That means sdhci might have flagged need_retune, since it's been >> runtime PM suspended. And for those scenarios I guess we really need >> to do a re-tune prior sending the sleep command, right? > > Yes, although that is how it works. Ohh, you are one step ahead of me. Good! :-) > > Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold() > but after one of the revisions I found that only one was needed. I stuck > with the mmc_retune_hold() name because it doesn't necessarily cause a > re-tune, but only if the hold count was zero and a retune is needed. > >> >> Earlier I only had the re-tune timer in mind, which is why I was less >> restrictive and suggesting you to add hold/disable. Sorry about that. >> >> Now, with the above in mind I believe you have similar issues with >> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6 >> (mmc: core: Hold re-tuning during erase commands). And that's because >> there are cases when the switch/erase commands are the first commands >> sent, after the sdhci host has been runtime PM suspended. I guess we >> need a way to make sure we don't hold re-tune for these cases. >> >> An option to deal with that is to use a separate flag set by host >> drivers, though the mmc_needs_retune() API and let that one override >> another. >> >> Forgive me for pushing you back and forth for how to do this, but it > > Not a problem. Thanks for persevering. > >> seems like we still have some outstanding issues to resolve. So that then more or less leaves us with one outstanding issue. The SDIO irq wakeup scenario. How will that work for sdhci? Your suggestion is to hold re-tune for the SDIO wakeup command. If I understand correct that could be overridden when the host flags need_retune from its runtime PM suspend callback, right? That then mean that the re-tuning will be done prior sending the wakeup command? That wouldn't work, unless the re-tune command also act as wakeup, which I doubt. If I _haven't_ understand correctly and you mean that the SDIO wakeup command shall be invoked prior re-tuning is done; that would mean that SDHCI will send a command to the card without first satisfying its need for a re-tune. And that wouldn't work either, right? So then the only solution for SDHCI would be to prevent it from being runtime PM suspended when configured for SDIO. Urgh, that's really bad. Comments? Kind regards Uffe > > > > -- > 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 -- 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 21/04/15 14:53, Ulf Hansson wrote: > On 21 April 2015 at 13:00, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 21/04/15 12:42, Ulf Hansson wrote: >>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> Currently "mmc sleep" is used before power off and >>>> is not paired with waking up. Nevertheless hold >>>> re-tuning. >>>> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>> --- >>>> drivers/mmc/core/mmc.c | 14 +++++++++++--- >>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>> index f36c76f..daf9954 100644 >>>> --- a/drivers/mmc/core/mmc.c >>>> +++ b/drivers/mmc/core/mmc.c >>>> @@ -21,6 +21,7 @@ >>>> #include <linux/mmc/mmc.h> >>>> >>>> #include "core.h" >>>> +#include "host.h" >>>> #include "bus.h" >>>> #include "mmc_ops.h" >>>> #include "sd_ops.h" >>>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >>>> return (card && card->ext_csd.rev >= 3); >>>> } >>>> >>>> +/* If necessary, callers must hold re-tuning */ >>>> static int mmc_sleep(struct mmc_host *host) >>>> { >>>> struct mmc_command cmd = {0}; >>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>>> int err = 0; >>>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >>>> EXT_CSD_POWER_OFF_LONG; >>>> + bool retune_release = false; >>>> >>>> BUG_ON(!host); >>>> BUG_ON(!host->card); >>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>>> goto out; >>>> >>>> if (mmc_can_poweroff_notify(host->card) && >>>> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >>>> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >>>> err = mmc_poweroff_notify(host->card, notify_type); >>>> - else if (mmc_can_sleep(host->card)) >>>> + } else if (mmc_can_sleep(host->card)) { >>>> + mmc_retune_hold(host); >>>> err = mmc_sleep(host); >>>> - else if (!mmc_host_is_spi(host)) >>>> + } else if (!mmc_host_is_spi(host)) { >>>> err = mmc_deselect_cards(host); >>>> + } >>>> >>>> if (!err) { >>>> mmc_power_off(host); >>>> mmc_card_set_suspended(host->card); >>>> } >>>> + >>>> + if (retune_release) >>>> + mmc_retune_release(host); >>>> out: >>>> mmc_release_host(host); >>>> return err; >>>> -- >>>> 1.9.1 >>>> >>> >>> According to our previous discussions I have given this some more thinking. >>> >>> I don't think we can allow to hold/disable re-tune in this path at >>> all. That's because we are claiming the host here and the sleep >>> command might then be the first command we invoke during the system PM >>> sequence. >>> >>> That means sdhci might have flagged need_retune, since it's been >>> runtime PM suspended. And for those scenarios I guess we really need >>> to do a re-tune prior sending the sleep command, right? >> >> Yes, although that is how it works. > > Ohh, you are one step ahead of me. Good! :-) > >> >> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold() >> but after one of the revisions I found that only one was needed. I stuck >> with the mmc_retune_hold() name because it doesn't necessarily cause a >> re-tune, but only if the hold count was zero and a retune is needed. >> >>> >>> Earlier I only had the re-tune timer in mind, which is why I was less >>> restrictive and suggesting you to add hold/disable. Sorry about that. >>> >>> Now, with the above in mind I believe you have similar issues with >>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6 >>> (mmc: core: Hold re-tuning during erase commands). And that's because >>> there are cases when the switch/erase commands are the first commands >>> sent, after the sdhci host has been runtime PM suspended. I guess we >>> need a way to make sure we don't hold re-tune for these cases. >>> >>> An option to deal with that is to use a separate flag set by host >>> drivers, though the mmc_needs_retune() API and let that one override >>> another. >>> >>> Forgive me for pushing you back and forth for how to do this, but it >> >> Not a problem. Thanks for persevering. >> >>> seems like we still have some outstanding issues to resolve. > > So that then more or less leaves us with one outstanding issue. The > SDIO irq wakeup scenario. > > How will that work for sdhci? > > Your suggestion is to hold re-tune for the SDIO wakeup command. If I > understand correct that could be overridden when the host flags > need_retune from its runtime PM suspend callback, right? > > That then mean that the re-tuning will be done prior sending the > wakeup command? That wouldn't work, unless the re-tune command also > act as wakeup, which I doubt. The wakeup command has to come first. > > If I _haven't_ understand correctly and you mean that the SDIO wakeup > command shall be invoked prior re-tuning is done; that would mean that > SDHCI will send a command to the card without first satisfying its > need for a re-tune. And that wouldn't work either, right? My understanding is that the wakeup command will still work but there might be a CRC error. Need Arend to comment on this since it is his driver we are talking about. So the plan would be: - re-tuning hold_count is incremented - wakeup command is issued (and no re-tuning is done) - errors are ignored - re-tuning hold_count is decremented - continue as normal, re-tuning before the next request as needed > > So then the only solution for SDHCI would be to prevent it from being > runtime PM suspended when configured for SDIO. Urgh, that's really > bad. Yes that would defeat the point of sleeping. -- 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 04/21/15 14:26, Adrian Hunter wrote: > On 21/04/15 14:53, Ulf Hansson wrote: >> On 21 April 2015 at 13:00, Adrian Hunter<adrian.hunter@intel.com> wrote: >>> On 21/04/15 12:42, Ulf Hansson wrote: >>>> On 20 April 2015 at 14:09, Adrian Hunter<adrian.hunter@intel.com> wrote: >>>>> Currently "mmc sleep" is used before power off and >>>>> is not paired with waking up. Nevertheless hold >>>>> re-tuning. >>>>> >>>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com> >>>>> --- >>>>> drivers/mmc/core/mmc.c | 14 +++++++++++--- >>>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>>> index f36c76f..daf9954 100644 >>>>> --- a/drivers/mmc/core/mmc.c >>>>> +++ b/drivers/mmc/core/mmc.c >>>>> @@ -21,6 +21,7 @@ >>>>> #include<linux/mmc/mmc.h> >>>>> >>>>> #include "core.h" >>>>> +#include "host.h" >>>>> #include "bus.h" >>>>> #include "mmc_ops.h" >>>>> #include "sd_ops.h" >>>>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >>>>> return (card&& card->ext_csd.rev>= 3); >>>>> } >>>>> >>>>> +/* If necessary, callers must hold re-tuning */ >>>>> static int mmc_sleep(struct mmc_host *host) >>>>> { >>>>> struct mmc_command cmd = {0}; >>>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>>>> int err = 0; >>>>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >>>>> EXT_CSD_POWER_OFF_LONG; >>>>> + bool retune_release = false; >>>>> >>>>> BUG_ON(!host); >>>>> BUG_ON(!host->card); >>>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>>>> goto out; >>>>> >>>>> if (mmc_can_poweroff_notify(host->card)&& >>>>> - ((host->caps2& MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >>>>> + ((host->caps2& MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >>>>> err = mmc_poweroff_notify(host->card, notify_type); >>>>> - else if (mmc_can_sleep(host->card)) >>>>> + } else if (mmc_can_sleep(host->card)) { >>>>> + mmc_retune_hold(host); >>>>> err = mmc_sleep(host); >>>>> - else if (!mmc_host_is_spi(host)) >>>>> + } else if (!mmc_host_is_spi(host)) { >>>>> err = mmc_deselect_cards(host); >>>>> + } >>>>> >>>>> if (!err) { >>>>> mmc_power_off(host); >>>>> mmc_card_set_suspended(host->card); >>>>> } >>>>> + >>>>> + if (retune_release) >>>>> + mmc_retune_release(host); >>>>> out: >>>>> mmc_release_host(host); >>>>> return err; >>>>> -- >>>>> 1.9.1 >>>>> >>>> >>>> According to our previous discussions I have given this some more thinking. >>>> >>>> I don't think we can allow to hold/disable re-tune in this path at >>>> all. That's because we are claiming the host here and the sleep >>>> command might then be the first command we invoke during the system PM >>>> sequence. >>>> >>>> That means sdhci might have flagged need_retune, since it's been >>>> runtime PM suspended. And for those scenarios I guess we really need >>>> to do a re-tune prior sending the sleep command, right? >>> >>> Yes, although that is how it works. >> >> Ohh, you are one step ahead of me. Good! :-) >> >>> >>> Previously I had two functions mmc_retune_hold() and mmc_retune_and_hold() >>> but after one of the revisions I found that only one was needed. I stuck >>> with the mmc_retune_hold() name because it doesn't necessarily cause a >>> re-tune, but only if the hold count was zero and a retune is needed. >>> >>>> >>>> Earlier I only had the re-tune timer in mind, which is why I was less >>>> restrictive and suggesting you to add hold/disable. Sorry about that. >>>> >>>> Now, with the above in mind I believe you have similar issues with >>>> patch5 (mmc: core: Hold re-tuning during switch commands) and patch6 >>>> (mmc: core: Hold re-tuning during erase commands). And that's because >>>> there are cases when the switch/erase commands are the first commands >>>> sent, after the sdhci host has been runtime PM suspended. I guess we >>>> need a way to make sure we don't hold re-tune for these cases. >>>> >>>> An option to deal with that is to use a separate flag set by host >>>> drivers, though the mmc_needs_retune() API and let that one override >>>> another. >>>> >>>> Forgive me for pushing you back and forth for how to do this, but it >>> >>> Not a problem. Thanks for persevering. >>> >>>> seems like we still have some outstanding issues to resolve. >> >> So that then more or less leaves us with one outstanding issue. The >> SDIO irq wakeup scenario. >> >> How will that work for sdhci? >> >> Your suggestion is to hold re-tune for the SDIO wakeup command. If I >> understand correct that could be overridden when the host flags >> need_retune from its runtime PM suspend callback, right? >> >> That then mean that the re-tuning will be done prior sending the >> wakeup command? That wouldn't work, unless the re-tune command also >> act as wakeup, which I doubt. > > The wakeup command has to come first. > >> >> If I _haven't_ understand correctly and you mean that the SDIO wakeup >> command shall be invoked prior re-tuning is done; that would mean that >> SDHCI will send a command to the card without first satisfying its >> need for a re-tune. And that wouldn't work either, right? > > My understanding is that the wakeup command will still work but there might > be a CRC error. > > Need Arend to comment on this since it is his driver we are talking about. Are we? > So the plan would be: > - re-tuning hold_count is incremented > - wakeup command is issued (and no re-tuning is done) > - errors are ignored > - re-tuning hold_count is decremented > - continue as normal, re-tuning before the next request as needed > >> >> So then the only solution for SDHCI would be to prevent it from being >> runtime PM suspended when configured for SDIO. Urgh, that's really >> bad. > > Yes that would defeat the point of sleeping. I recently submitted a patch in brcmfmac to disable runtime pm for the SDIO host controller as it interfered with communication between driver and device, ie. driver send request to device but response is never received because runtime-pm kicked in. Our sdio func driver does not provide runtime-pm (yet) and figured using pm_runtime_forbid() was the only way to let the host controller know this fact. Regards, Arend -- 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 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: > Currently "mmc sleep" is used before power off and > is not paired with waking up. Nevertheless hold > re-tuning. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/mmc.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index f36c76f..daf9954 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -21,6 +21,7 @@ > #include <linux/mmc/mmc.h> > > #include "core.h" > +#include "host.h" > #include "bus.h" > #include "mmc_ops.h" > #include "sd_ops.h" > @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) > return (card && card->ext_csd.rev >= 3); > } > > +/* If necessary, callers must hold re-tuning */ Remove this comment. > static int mmc_sleep(struct mmc_host *host) > { > struct mmc_command cmd = {0}; > @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > int err = 0; > unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : > EXT_CSD_POWER_OFF_LONG; > + bool retune_release = false; > > BUG_ON(!host); > BUG_ON(!host->card); > @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > goto out; > > if (mmc_can_poweroff_notify(host->card) && > - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) > + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { > err = mmc_poweroff_notify(host->card, notify_type); > - else if (mmc_can_sleep(host->card)) > + } else if (mmc_can_sleep(host->card)) { > + mmc_retune_hold(host); > err = mmc_sleep(host); > - else if (!mmc_host_is_spi(host)) > + } else if (!mmc_host_is_spi(host)) { > err = mmc_deselect_cards(host); > + } > > if (!err) { > mmc_power_off(host); > mmc_card_set_suspended(host->card); > } > + > + if (retune_release) > + mmc_retune_release(host); > out: > mmc_release_host(host); > return err; Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would like you to move that handling into mmc_sleep(). The code should be easier and it becomes more clear that it's because of a command sequence. I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() and then mmc_retune_release() just after, in mmc_sleep(). That should work, right!? Kind regards Uffe -- 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 04/05/15 16:44, Ulf Hansson wrote: > On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: >> Currently "mmc sleep" is used before power off and >> is not paired with waking up. Nevertheless hold >> re-tuning. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/mmc/core/mmc.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >> index f36c76f..daf9954 100644 >> --- a/drivers/mmc/core/mmc.c >> +++ b/drivers/mmc/core/mmc.c >> @@ -21,6 +21,7 @@ >> #include <linux/mmc/mmc.h> >> >> #include "core.h" >> +#include "host.h" >> #include "bus.h" >> #include "mmc_ops.h" >> #include "sd_ops.h" >> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >> return (card && card->ext_csd.rev >= 3); >> } >> >> +/* If necessary, callers must hold re-tuning */ > > Remove this comment. > >> static int mmc_sleep(struct mmc_host *host) >> { >> struct mmc_command cmd = {0}; >> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >> int err = 0; >> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >> EXT_CSD_POWER_OFF_LONG; >> + bool retune_release = false; >> >> BUG_ON(!host); >> BUG_ON(!host->card); >> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >> goto out; >> >> if (mmc_can_poweroff_notify(host->card) && >> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >> err = mmc_poweroff_notify(host->card, notify_type); >> - else if (mmc_can_sleep(host->card)) >> + } else if (mmc_can_sleep(host->card)) { >> + mmc_retune_hold(host); >> err = mmc_sleep(host); >> - else if (!mmc_host_is_spi(host)) >> + } else if (!mmc_host_is_spi(host)) { >> err = mmc_deselect_cards(host); >> + } >> >> if (!err) { >> mmc_power_off(host); >> mmc_card_set_suspended(host->card); >> } >> + >> + if (retune_release) >> + mmc_retune_release(host); >> out: >> mmc_release_host(host); >> return err; > > Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would > like you to move that handling into mmc_sleep(). The code should be > easier and it becomes more clear that it's because of a command > sequence. > > I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() > and then mmc_retune_release() just after, in mmc_sleep(). That should > work, right!? That would be the same as holding re-tuning for that request, which is what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() is redundant. The options for the caller are: 1) hold re-tuning put emmc to sleep later wake up emmc release re-tuning 2) put emmc to sleep later increment hold_count wake up emmc ignoring CRC errors release re-tuning But there is no wake-up function and the suspend path is using an unbalanced mmc_sleep i.e. no corresponding wake up. So that leaves what is happening now i.e. a comment plus explicit hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() know to take mmc_sleep re-tuning requirements into account. -- 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 6 May 2015 at 10:39, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 04/05/15 16:44, Ulf Hansson wrote: >> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> Currently "mmc sleep" is used before power off and >>> is not paired with waking up. Nevertheless hold >>> re-tuning. >>> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>> --- >>> drivers/mmc/core/mmc.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index f36c76f..daf9954 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -21,6 +21,7 @@ >>> #include <linux/mmc/mmc.h> >>> >>> #include "core.h" >>> +#include "host.h" >>> #include "bus.h" >>> #include "mmc_ops.h" >>> #include "sd_ops.h" >>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >>> return (card && card->ext_csd.rev >= 3); >>> } >>> >>> +/* If necessary, callers must hold re-tuning */ >> >> Remove this comment. >> >>> static int mmc_sleep(struct mmc_host *host) >>> { >>> struct mmc_command cmd = {0}; >>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>> int err = 0; >>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >>> EXT_CSD_POWER_OFF_LONG; >>> + bool retune_release = false; >>> >>> BUG_ON(!host); >>> BUG_ON(!host->card); >>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>> goto out; >>> >>> if (mmc_can_poweroff_notify(host->card) && >>> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >>> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >>> err = mmc_poweroff_notify(host->card, notify_type); >>> - else if (mmc_can_sleep(host->card)) >>> + } else if (mmc_can_sleep(host->card)) { >>> + mmc_retune_hold(host); >>> err = mmc_sleep(host); >>> - else if (!mmc_host_is_spi(host)) >>> + } else if (!mmc_host_is_spi(host)) { >>> err = mmc_deselect_cards(host); >>> + } >>> >>> if (!err) { >>> mmc_power_off(host); >>> mmc_card_set_suspended(host->card); >>> } >>> + >>> + if (retune_release) >>> + mmc_retune_release(host); >>> out: >>> mmc_release_host(host); >>> return err; >> >> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >> like you to move that handling into mmc_sleep(). The code should be >> easier and it becomes more clear that it's because of a command >> sequence. >> >> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >> and then mmc_retune_release() just after, in mmc_sleep(). That should >> work, right!? > > That would be the same as holding re-tuning for that request, which is > what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() > is redundant. I don't understand your point, sorry. Anyway, my proposal didn't quite work, which is due to that mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If there had been only one try, I thought it could be okay to have that command to be preceded by a re-tune. Anyway, I would like you to move the mmc_retune_hold|release() calls into the mmc_sleep() function. > > The options for the caller are: > > 1) > hold re-tuning > put emmc to sleep > later wake up emmc > release re-tuning > > 2) > put emmc to sleep > later increment hold_count > wake up emmc ignoring CRC errors > release re-tuning > > But there is no wake-up function and the suspend path is using an unbalanced > mmc_sleep i.e. no corresponding wake up. > > So that leaves what is happening now i.e. a comment plus explicit > hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() > know to take mmc_sleep re-tuning requirements into account. Why all this complexity? mmc_power_off() is called in _mmc_suspend(), that will eventually disable re-tune. Thus re-tuning will be prevented for commands/requests during the system PM resume sequence, until the card has been fully re-initialized (and a tuning sequence done). Isn't that sufficient? Kind regards Uffe -- 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 06/05/15 12:32, Ulf Hansson wrote: > On 6 May 2015 at 10:39, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 04/05/15 16:44, Ulf Hansson wrote: >>> On 20 April 2015 at 14:09, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> Currently "mmc sleep" is used before power off and >>>> is not paired with waking up. Nevertheless hold >>>> re-tuning. >>>> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>> --- >>>> drivers/mmc/core/mmc.c | 14 +++++++++++--- >>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>> index f36c76f..daf9954 100644 >>>> --- a/drivers/mmc/core/mmc.c >>>> +++ b/drivers/mmc/core/mmc.c >>>> @@ -21,6 +21,7 @@ >>>> #include <linux/mmc/mmc.h> >>>> >>>> #include "core.h" >>>> +#include "host.h" >>>> #include "bus.h" >>>> #include "mmc_ops.h" >>>> #include "sd_ops.h" >>>> @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) >>>> return (card && card->ext_csd.rev >= 3); >>>> } >>>> >>>> +/* If necessary, callers must hold re-tuning */ >>> >>> Remove this comment. >>> >>>> static int mmc_sleep(struct mmc_host *host) >>>> { >>>> struct mmc_command cmd = {0}; >>>> @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>>> int err = 0; >>>> unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : >>>> EXT_CSD_POWER_OFF_LONG; >>>> + bool retune_release = false; >>>> >>>> BUG_ON(!host); >>>> BUG_ON(!host->card); >>>> @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) >>>> goto out; >>>> >>>> if (mmc_can_poweroff_notify(host->card) && >>>> - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) >>>> + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { >>>> err = mmc_poweroff_notify(host->card, notify_type); >>>> - else if (mmc_can_sleep(host->card)) >>>> + } else if (mmc_can_sleep(host->card)) { >>>> + mmc_retune_hold(host); >>>> err = mmc_sleep(host); >>>> - else if (!mmc_host_is_spi(host)) >>>> + } else if (!mmc_host_is_spi(host)) { >>>> err = mmc_deselect_cards(host); >>>> + } >>>> >>>> if (!err) { >>>> mmc_power_off(host); >>>> mmc_card_set_suspended(host->card); >>>> } >>>> + >>>> + if (retune_release) >>>> + mmc_retune_release(host); >>>> out: >>>> mmc_release_host(host); >>>> return err; >>> >>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >>> like you to move that handling into mmc_sleep(). The code should be >>> easier and it becomes more clear that it's because of a command >>> sequence. >>> >>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >>> and then mmc_retune_release() just after, in mmc_sleep(). That should >>> work, right!? >> >> That would be the same as holding re-tuning for that request, which is >> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() >> is redundant. > > I don't understand your point, sorry. mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req() which calls mmc_start_request() which calls mmc_retune_hold() Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls mmc_retune_release(). So mmc_wait_for_cmd() (with no retries) has the same effect as mmc_retune_hold() mmc_wait_for_cmd() mmc_retune_release() > > Anyway, my proposal didn't quite work, which is due to that > mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If > there had been only one try, I thought it could be okay to have that > command to be preceded by a re-tune. > > Anyway, I would like you to move the mmc_retune_hold|release() calls > into the mmc_sleep() function. That would have no effect as explained above. > >> >> The options for the caller are: >> >> 1) >> hold re-tuning >> put emmc to sleep >> later wake up emmc >> release re-tuning >> >> 2) >> put emmc to sleep >> later increment hold_count >> wake up emmc ignoring CRC errors >> release re-tuning >> >> But there is no wake-up function and the suspend path is using an unbalanced >> mmc_sleep i.e. no corresponding wake up. >> >> So that leaves what is happening now i.e. a comment plus explicit >> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() >> know to take mmc_sleep re-tuning requirements into account. > > Why all this complexity? > > mmc_power_off() is called in _mmc_suspend(), that will eventually > disable re-tune. Thus re-tuning will be prevented for > commands/requests during the system PM resume sequence, until the card > has been fully re-initialized (and a tuning sequence done). Isn't that > sufficient? Yes my original patch did not have any of that complexity. I added it in response to our discussions. As you wrote, _mmc_suspend() does not need to do anything with retuning because mmc_sleep() is followed by mmc_power_off(). The original patch added a comment to mmc_sleep() and that was all. That would still be the best approach. -- 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
[...] >>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >>>> like you to move that handling into mmc_sleep(). The code should be >>>> easier and it becomes more clear that it's because of a command >>>> sequence. >>>> >>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >>>> and then mmc_retune_release() just after, in mmc_sleep(). That should >>>> work, right!? >>> >>> That would be the same as holding re-tuning for that request, which is >>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() >>> is redundant. >> >> I don't understand your point, sorry. > > mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req() > which calls mmc_start_request() which calls mmc_retune_hold() > > Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls > mmc_retune_release(). > > So > mmc_wait_for_cmd() (with no retries) > has the same effect as > mmc_retune_hold() > mmc_wait_for_cmd() > mmc_retune_release() > Huh, you are right - again. There have been a couple of iterations of this patchset, I don't recall why we need to hold retune for all requests? It seems awkward. Shouldn't we just hold retune for those requests that needs it? >> >> Anyway, my proposal didn't quite work, which is due to that >> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If >> there had been only one try, I thought it could be okay to have that >> command to be preceded by a re-tune. >> >> Anyway, I would like you to move the mmc_retune_hold|release() calls >> into the mmc_sleep() function. > > That would have no effect as explained above. Then why did you add it to the _mmc_suspend() function? What am I missing here? > >> >>> >>> The options for the caller are: >>> >>> 1) >>> hold re-tuning >>> put emmc to sleep >>> later wake up emmc >>> release re-tuning >>> >>> 2) >>> put emmc to sleep >>> later increment hold_count >>> wake up emmc ignoring CRC errors >>> release re-tuning >>> >>> But there is no wake-up function and the suspend path is using an unbalanced >>> mmc_sleep i.e. no corresponding wake up. >>> >>> So that leaves what is happening now i.e. a comment plus explicit >>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() >>> know to take mmc_sleep re-tuning requirements into account. >> >> Why all this complexity? >> >> mmc_power_off() is called in _mmc_suspend(), that will eventually >> disable re-tune. Thus re-tuning will be prevented for >> commands/requests during the system PM resume sequence, until the card >> has been fully re-initialized (and a tuning sequence done). Isn't that >> sufficient? > > Yes my original patch did not have any of that complexity. I added it in > response to our discussions. > > As you wrote, _mmc_suspend() does not need to do anything with retuning > because mmc_sleep() is followed by mmc_power_off(). > > The original patch added a comment to mmc_sleep() and that was all. That > would still be the best approach. > What I had in mind was that the re-tune timer could time out in the middle of the _mmc_suspend() sequence. If that happens in-between mmc_deselect_cards() and when the CMD5 is to be sent, in mmc_sleep() - we must not allow a re-tune sequence. Unless holding re-tune here, how is that prevented? Kind regards Uffe -- 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 06/05/15 14:36, Ulf Hansson wrote: > [...] > >>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >>>>> like you to move that handling into mmc_sleep(). The code should be >>>>> easier and it becomes more clear that it's because of a command >>>>> sequence. >>>>> >>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should >>>>> work, right!? >>>> >>>> That would be the same as holding re-tuning for that request, which is >>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() >>>> is redundant. >>> >>> I don't understand your point, sorry. >> >> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req() >> which calls mmc_start_request() which calls mmc_retune_hold() >> >> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls >> mmc_retune_release(). >> >> So >> mmc_wait_for_cmd() (with no retries) >> has the same effect as >> mmc_retune_hold() >> mmc_wait_for_cmd() >> mmc_retune_release() >> > > Huh, you are right - again. > > There have been a couple of iterations of this patchset, I don't > recall why we need to hold retune for all requests? It seems awkward. > Shouldn't we just hold retune for those requests that needs it? For data requests (which also call __mmc_start_req()) there is the possibility that a 'write' is not finished and is polled with CMD13. So re-tuning is held to avoid conflicting with the busy state. It also aids controlling when re-tuning happens in the recovery path i.e. we have a go at getting the status first and if that doesn't work first time, then re-tune if needed. Also mmc_retune_hold() does not only hold retuning, it also causes re-tuning to happen if the hold_count was zero, so it does "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning" > >>> >>> Anyway, my proposal didn't quite work, which is due to that >>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If >>> there had been only one try, I thought it could be okay to have that >>> command to be preceded by a re-tune. >>> >>> Anyway, I would like you to move the mmc_retune_hold|release() calls >>> into the mmc_sleep() function. >> >> That would have no effect as explained above. > > Then why did you add it to the _mmc_suspend() function? What am I missing here? It was added in response to our discussions. It was not in my original patches. I can take it out. > >> >>> >>>> >>>> The options for the caller are: >>>> >>>> 1) >>>> hold re-tuning >>>> put emmc to sleep >>>> later wake up emmc >>>> release re-tuning >>>> >>>> 2) >>>> put emmc to sleep >>>> later increment hold_count >>>> wake up emmc ignoring CRC errors >>>> release re-tuning >>>> >>>> But there is no wake-up function and the suspend path is using an unbalanced >>>> mmc_sleep i.e. no corresponding wake up. >>>> >>>> So that leaves what is happening now i.e. a comment plus explicit >>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() >>>> know to take mmc_sleep re-tuning requirements into account. >>> >>> Why all this complexity? >>> >>> mmc_power_off() is called in _mmc_suspend(), that will eventually >>> disable re-tune. Thus re-tuning will be prevented for >>> commands/requests during the system PM resume sequence, until the card >>> has been fully re-initialized (and a tuning sequence done). Isn't that >>> sufficient? >> >> Yes my original patch did not have any of that complexity. I added it in >> response to our discussions. >> >> As you wrote, _mmc_suspend() does not need to do anything with retuning >> because mmc_sleep() is followed by mmc_power_off(). >> >> The original patch added a comment to mmc_sleep() and that was all. That >> would still be the best approach. >> > > What I had in mind was that the re-tune timer could time out in the > middle of the _mmc_suspend() sequence. > > If that happens in-between mmc_deselect_cards() and when the CMD5 is > to be sent, in mmc_sleep() - we must not allow a re-tune sequence. > Unless holding re-tune here, how is that prevented? Oh yes, I have overlooked that re-tuning can't be done on a de-selected card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to think about the error handling. It looks broken now anyway since it doesn't reselect the card in the error path. -- 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 6 May 2015 at 14:42, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 06/05/15 14:36, Ulf Hansson wrote: >> [...] >> >>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >>>>>> like you to move that handling into mmc_sleep(). The code should be >>>>>> easier and it becomes more clear that it's because of a command >>>>>> sequence. >>>>>> >>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should >>>>>> work, right!? >>>>> >>>>> That would be the same as holding re-tuning for that request, which is >>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() >>>>> is redundant. >>>> >>>> I don't understand your point, sorry. >>> >>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req() >>> which calls mmc_start_request() which calls mmc_retune_hold() >>> >>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls >>> mmc_retune_release(). >>> >>> So >>> mmc_wait_for_cmd() (with no retries) >>> has the same effect as >>> mmc_retune_hold() >>> mmc_wait_for_cmd() >>> mmc_retune_release() >>> >> >> Huh, you are right - again. >> >> There have been a couple of iterations of this patchset, I don't >> recall why we need to hold retune for all requests? It seems awkward. >> Shouldn't we just hold retune for those requests that needs it? > > For data requests (which also call __mmc_start_req()) there is the > possibility that a 'write' is not finished and is polled with CMD13. > So re-tuning is held to avoid conflicting with the busy state. > It also aids controlling when re-tuning happens in the recovery path > i.e. we have a go at getting the status first and if that doesn't > work first time, then re-tune if needed. > > Also mmc_retune_hold() does not only hold retuning, it also causes > re-tuning to happen if the hold_count was zero, so it does > "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning" Hmm, is there anyway we can make this easier to understand in the code path and maybe clarify via the name of functions/APIs you add? Could we have a state variable instead of bunch of int variables? Since I apparently have a bit hard time to understand how this actually works, I am a bit concerned about the maintenance of it. :-) Anyway, if you can't find any better option - I will accept it as is. > >> >>>> >>>> Anyway, my proposal didn't quite work, which is due to that >>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If >>>> there had been only one try, I thought it could be okay to have that >>>> command to be preceded by a re-tune. >>>> >>>> Anyway, I would like you to move the mmc_retune_hold|release() calls >>>> into the mmc_sleep() function. >>> >>> That would have no effect as explained above. >> >> Then why did you add it to the _mmc_suspend() function? What am I missing here? > > It was added in response to our discussions. It was not in my original > patches. I can take it out. > >> >>> >>>> >>>>> >>>>> The options for the caller are: >>>>> >>>>> 1) >>>>> hold re-tuning >>>>> put emmc to sleep >>>>> later wake up emmc >>>>> release re-tuning >>>>> >>>>> 2) >>>>> put emmc to sleep >>>>> later increment hold_count >>>>> wake up emmc ignoring CRC errors >>>>> release re-tuning >>>>> >>>>> But there is no wake-up function and the suspend path is using an unbalanced >>>>> mmc_sleep i.e. no corresponding wake up. >>>>> >>>>> So that leaves what is happening now i.e. a comment plus explicit >>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() >>>>> know to take mmc_sleep re-tuning requirements into account. >>>> >>>> Why all this complexity? >>>> >>>> mmc_power_off() is called in _mmc_suspend(), that will eventually >>>> disable re-tune. Thus re-tuning will be prevented for >>>> commands/requests during the system PM resume sequence, until the card >>>> has been fully re-initialized (and a tuning sequence done). Isn't that >>>> sufficient? >>> >>> Yes my original patch did not have any of that complexity. I added it in >>> response to our discussions. >>> >>> As you wrote, _mmc_suspend() does not need to do anything with retuning >>> because mmc_sleep() is followed by mmc_power_off(). >>> >>> The original patch added a comment to mmc_sleep() and that was all. That >>> would still be the best approach. >>> >> >> What I had in mind was that the re-tune timer could time out in the >> middle of the _mmc_suspend() sequence. >> >> If that happens in-between mmc_deselect_cards() and when the CMD5 is >> to be sent, in mmc_sleep() - we must not allow a re-tune sequence. >> Unless holding re-tune here, how is that prevented? > > Oh yes, I have overlooked that re-tuning can't be done on a de-selected > card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to > think about the error handling. It looks broken now anyway since it doesn't > reselect the card in the error path. > I suggest you don't bother about the error handling for now, we can take that separately. Kind regards Uffe -- 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 06/05/15 16:21, Ulf Hansson wrote: > On 6 May 2015 at 14:42, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 06/05/15 14:36, Ulf Hansson wrote: >>> [...] >>> >>>>>>> Instead of add mmc_retune_hold|release() to _mmc_suspend(), I would >>>>>>> like you to move that handling into mmc_sleep(). The code should be >>>>>>> easier and it becomes more clear that it's because of a command >>>>>>> sequence. >>>>>>> >>>>>>> I think mmc_retune_hold() should be invoked before mmc_wait_for_cmd() >>>>>>> and then mmc_retune_release() just after, in mmc_sleep(). That should >>>>>>> work, right!? >>>>>> >>>>>> That would be the same as holding re-tuning for that request, which is >>>>>> what already happens i.e. adding hold()/release() around mmc_wait_for_cmd() >>>>>> is redundant. >>>>> >>>>> I don't understand your point, sorry. >>>> >>>> mmc_wait_for_cmd() calls mmc_wait_for_req() which calls __mmc_start_req() >>>> which calls mmc_start_request() which calls mmc_retune_hold() >>>> >>>> Then mmc_wait_for_req() calls mmc_wait_for_req_done() which calls >>>> mmc_retune_release(). >>>> >>>> So >>>> mmc_wait_for_cmd() (with no retries) >>>> has the same effect as >>>> mmc_retune_hold() >>>> mmc_wait_for_cmd() >>>> mmc_retune_release() >>>> >>> >>> Huh, you are right - again. >>> >>> There have been a couple of iterations of this patchset, I don't >>> recall why we need to hold retune for all requests? It seems awkward. >>> Shouldn't we just hold retune for those requests that needs it? >> >> For data requests (which also call __mmc_start_req()) there is the >> possibility that a 'write' is not finished and is polled with CMD13. >> So re-tuning is held to avoid conflicting with the busy state. >> It also aids controlling when re-tuning happens in the recovery path >> i.e. we have a go at getting the status first and if that doesn't >> work first time, then re-tune if needed. >> >> Also mmc_retune_hold() does not only hold retuning, it also causes >> re-tuning to happen if the hold_count was zero, so it does >> "make-retuning-happen-if-needed-and-not-already-held-and-then-hold-retuning" > > Hmm, is there anyway we can make this easier to understand in the code > path and maybe clarify via the name of functions/APIs you add? Could > we have a state variable instead of bunch of int variables? The ints are needed either to allow nesting or atomic update. > > Since I apparently have a bit hard time to understand how this > actually works, I am a bit concerned about the maintenance of it. :-) There are only a few things to remember: 1. Re-tuning can happen before every request i.e. inside mmc_wait_for_req() or mmc_start_req() 2. If you have several requests where re-tuning can't be done in between them, then you can put mmc_retune_hold() / mmc_retune_release() around them 3. A sleep state, like the brcm custom sleep state, might need to prevent re-tuning for the wakeup command > > Anyway, if you can't find any better option - I will accept it as is. > >> >>> >>>>> >>>>> Anyway, my proposal didn't quite work, which is due to that >>>>> mmc_deselect_cards() (invoked from mmc_sleep()) deals with retries. If >>>>> there had been only one try, I thought it could be okay to have that >>>>> command to be preceded by a re-tune. >>>>> >>>>> Anyway, I would like you to move the mmc_retune_hold|release() calls >>>>> into the mmc_sleep() function. >>>> >>>> That would have no effect as explained above. >>> >>> Then why did you add it to the _mmc_suspend() function? What am I missing here? >> >> It was added in response to our discussions. It was not in my original >> patches. I can take it out. >> >>> >>>> >>>>> >>>>>> >>>>>> The options for the caller are: >>>>>> >>>>>> 1) >>>>>> hold re-tuning >>>>>> put emmc to sleep >>>>>> later wake up emmc >>>>>> release re-tuning >>>>>> >>>>>> 2) >>>>>> put emmc to sleep >>>>>> later increment hold_count >>>>>> wake up emmc ignoring CRC errors >>>>>> release re-tuning >>>>>> >>>>>> But there is no wake-up function and the suspend path is using an unbalanced >>>>>> mmc_sleep i.e. no corresponding wake up. >>>>>> >>>>>> So that leaves what is happening now i.e. a comment plus explicit >>>>>> hold()/release() in _mmc_suspend() so that future changes to _mmc_suspend() >>>>>> know to take mmc_sleep re-tuning requirements into account. >>>>> >>>>> Why all this complexity? >>>>> >>>>> mmc_power_off() is called in _mmc_suspend(), that will eventually >>>>> disable re-tune. Thus re-tuning will be prevented for >>>>> commands/requests during the system PM resume sequence, until the card >>>>> has been fully re-initialized (and a tuning sequence done). Isn't that >>>>> sufficient? >>>> >>>> Yes my original patch did not have any of that complexity. I added it in >>>> response to our discussions. >>>> >>>> As you wrote, _mmc_suspend() does not need to do anything with retuning >>>> because mmc_sleep() is followed by mmc_power_off(). >>>> >>>> The original patch added a comment to mmc_sleep() and that was all. That >>>> would still be the best approach. >>>> >>> >>> What I had in mind was that the re-tune timer could time out in the >>> middle of the _mmc_suspend() sequence. >>> >>> If that happens in-between mmc_deselect_cards() and when the CMD5 is >>> to be sent, in mmc_sleep() - we must not allow a re-tune sequence. >>> Unless holding re-tune here, how is that prevented? >> >> Oh yes, I have overlooked that re-tuning can't be done on a de-selected >> card. So I will add mmc_retune_hold()/mmc_retune_release(). I will have to >> think about the error handling. It looks broken now anyway since it doesn't >> reselect the card in the error path. >> > > I suggest you don't bother about the error handling for now, we can > take that separately. Ok, thanks! -- 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/mmc.c b/drivers/mmc/core/mmc.c index f36c76f..daf9954 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -21,6 +21,7 @@ #include <linux/mmc/mmc.h> #include "core.h" +#include "host.h" #include "bus.h" #include "mmc_ops.h" #include "sd_ops.h" @@ -1504,6 +1505,7 @@ static int mmc_can_sleep(struct mmc_card *card) return (card && card->ext_csd.rev >= 3); } +/* If necessary, callers must hold re-tuning */ static int mmc_sleep(struct mmc_host *host) { struct mmc_command cmd = {0}; @@ -1631,6 +1633,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) int err = 0; unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : EXT_CSD_POWER_OFF_LONG; + bool retune_release = false; BUG_ON(!host); BUG_ON(!host->card); @@ -1651,17 +1654,22 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) goto out; if (mmc_can_poweroff_notify(host->card) && - ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend)) { err = mmc_poweroff_notify(host->card, notify_type); - else if (mmc_can_sleep(host->card)) + } else if (mmc_can_sleep(host->card)) { + mmc_retune_hold(host); err = mmc_sleep(host); - else if (!mmc_host_is_spi(host)) + } else if (!mmc_host_is_spi(host)) { err = mmc_deselect_cards(host); + } if (!err) { mmc_power_off(host); mmc_card_set_suspended(host->card); } + + if (retune_release) + mmc_retune_release(host); out: mmc_release_host(host); return err;
Currently "mmc sleep" is used before power off and is not paired with waking up. Nevertheless hold re-tuning. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/mmc.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)