| Message ID | 20250320140040.162416-3-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: core: Add support for graceful host removal for eMMC/SD | expand |
> To manage a graceful power-off of the eMMC card during platform shutdown, > the preferred option is to use the poweroff-notification command. > > Due to an earlier suspend request the eMMC may already have been properly > powered-off, hence we are sometimes leaving the eMMC in its current state. > However, in one case when the host has > MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND > set we may unnecessarily restore the power to the eMMC, let's avoid this. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/core/mmc.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > 3424bc9e20c5..400dd0449fec 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -2014,6 +2014,18 @@ static bool mmc_can_poweroff_notify(const > struct mmc_card *card) > (card->ext_csd.power_off_notification == > EXT_CSD_POWER_ON); } > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host, > + bool is_suspend) > +{ > + if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) > + return true; > + > + if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND && > is_suspend) > + return true; > + > + return !is_suspend; > +} > + > static int mmc_poweroff_notify(struct mmc_card *card, unsigned int > notify_type) { > unsigned int timeout = card->ext_csd.generic_cmd6_time; @@ - > 2124,8 +2136,7 @@ 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_IN_SUSPEND))) > + mmc_may_poweroff_notify(host, is_suspend)) > err = mmc_poweroff_notify(host->card, notify_type); > else if (mmc_can_sleep(host->card)) > err = mmc_sleep(host); > @@ -2191,7 +2202,7 @@ static int mmc_shutdown(struct mmc_host *host) > * before we can shutdown it properly. > */ > if (mmc_can_poweroff_notify(host->card) && > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > + !mmc_may_poweroff_notify(host, true)) I guess this deserve some extra documentation because: If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set, !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true. Thanks, Avri > err = _mmc_resume(host); > > if (!err) > -- > 2.43.0 >
> > +static bool mmc_may_poweroff_notify(const struct mmc_host *host, > > + bool is_suspend) Maybe add some comments about the difference between mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it super-obvious, so I will easily remember next year again :) > > if (mmc_can_poweroff_notify(host->card) && > > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > > + !mmc_may_poweroff_notify(host, true)) > I guess this deserve some extra documentation because: > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set, > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true. I agree, I neither get this. Another way to express my confusion is: Why do we set the 'is_suspend' flag to true in the shutdown function?
On Mon, 31 Mar 2025 at 10:21, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host, > > > + bool is_suspend) > > Maybe add some comments about the difference between > mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it > super-obvious, so I will easily remember next year again :) mmc_can_* functions are mostly about checking what the card is capable of. So mmc_can_poweroff_notify() should be consistent with the other similar functions. For eMMC power-off notifications in particular, it has become more complicated as we need to check the power-off scenario along with the host's capabilities, to understand what we should do. I am certainly open to another name than mmc_may_power_off_notify(), if that is what you are suggesting. Do you have a proposal? :-) > > > > if (mmc_can_poweroff_notify(host->card) && > > > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > > > + !mmc_may_poweroff_notify(host, true)) > > I guess this deserve some extra documentation because: > > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set, > > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true. Right. See more below. > > I agree, I neither get this. Another way to express my confusion is: Why > do we set the 'is_suspend' flag to true in the shutdown function? > I understand your concern and I agree that this is rather messy. Anyway, for shutdown, we set the is_suspend flag to false. The reasoning behind this is that during shutdown we know that the card will be fully powered-down (both vcc and vccq will be cut). In suspend/runtime_suspend, we don't really know as it depends on what the platform/host is capable of. If we can't do a full power-off (maybe just vcc can be cut), then we prefer the sleep command instead. I was hoping that patch3 should make this more clear (using an enum type), but I can try to add some comment(s) in the code to further clarify the policy. Thanks for reviewing and testing! Kind regards Uffe
Hi Ulf, > > > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host, > > > > + bool is_suspend) > > > > Maybe add some comments about the difference between > > mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it > > super-obvious, so I will easily remember next year again :) > > mmc_can_* functions are mostly about checking what the card is capable > of. So mmc_can_poweroff_notify() should be consistent with the other > similar functions. > > For eMMC power-off notifications in particular, it has become more > complicated as we need to check the power-off scenario along with the > host's capabilities, to understand what we should do. > > I am certainly open to another name than mmc_may_power_off_notify(), > if that is what you are suggesting. Do you have a proposal? :-) Initially, I didn't think of new names but some explanation in comments. But since you are mentioning a rename now, how about: mmc_card_can_poweroff_notify() and mmc_host_can_poweroff_notify()? Similar to the commit 32f18e596141 ("mmc: improve API to make clear hw_reset callback is for cards") where I renamed 'hw_reset' to 'card_hw_reset' for AFAICS similar reasons. > > > > if (mmc_can_poweroff_notify(host->card) && > > > > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > > > > + !mmc_may_poweroff_notify(host, true)) > > > I guess this deserve some extra documentation because: > > > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set, > > > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true. > > Right. See more below. > > > > > I agree, I neither get this. Another way to express my confusion is: Why > > do we set the 'is_suspend' flag to true in the shutdown function? > > > > I understand your concern and I agree that this is rather messy. > Anyway, for shutdown, we set the is_suspend flag to false. The > reasoning behind this is that during shutdown we know that the card > will be fully powered-down (both vcc and vccq will be cut). > > In suspend/runtime_suspend, we don't really know as it depends on what > the platform/host is capable of. If we can't do a full power-off > (maybe just vcc can be cut), then we prefer the sleep command instead. I do understand that. I don't see why this needs a change in the existing logic as Alan pointed out above. > I was hoping that patch3 should make this more clear (using an enum Sadly, it didn't. Using MMC_POWEROFF_SUSPEND first and then later MMC_POWEROFF_SHUTDOWN in mmc_shutdown() is still confusing. Do you want to return false in case none of the two PWR_CYCLE flags is set? > type), but I can try to add some comment(s) in the code to further > clarify the policy. Please do. All the best, Wolfram
On Mon, 31 Mar 2025 at 12:46, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Hi Ulf, > > > > > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host, > > > > > + bool is_suspend) > > > > > > Maybe add some comments about the difference between > > > mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it > > > super-obvious, so I will easily remember next year again :) > > > > mmc_can_* functions are mostly about checking what the card is capable > > of. So mmc_can_poweroff_notify() should be consistent with the other > > similar functions. > > > > For eMMC power-off notifications in particular, it has become more > > complicated as we need to check the power-off scenario along with the > > host's capabilities, to understand what we should do. > > > > I am certainly open to another name than mmc_may_power_off_notify(), > > if that is what you are suggesting. Do you have a proposal? :-) > > Initially, I didn't think of new names but some explanation in comments. > But since you are mentioning a rename now, how about: > > mmc_card_can_poweroff_notify() and mmc_host_can_poweroff_notify()? mmc_card_can_poweroff_notify() would not be consistent with all the other mmc_can_* helpers, so I rather stay with mmc_can_poweroff_notify(), for now. If you think a rename makes sense, I suggest we do that as a follow up and rename all the helpers. mmc_host_can_poweroff_notify() seems fine to me! > > Similar to the commit 32f18e596141 ("mmc: improve API to make clear > hw_reset callback is for cards") where I renamed 'hw_reset' to > 'card_hw_reset' for AFAICS similar reasons. > > > > > > if (mmc_can_poweroff_notify(host->card) && > > > > > - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) > > > > > + !mmc_may_poweroff_notify(host, true)) > > > > I guess this deserve some extra documentation because: > > > > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set, > > > > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true. > > > > Right. See more below. > > > > > > > > I agree, I neither get this. Another way to express my confusion is: Why > > > do we set the 'is_suspend' flag to true in the shutdown function? > > > > > > > I understand your concern and I agree that this is rather messy. > > Anyway, for shutdown, we set the is_suspend flag to false. The > > reasoning behind this is that during shutdown we know that the card > > will be fully powered-down (both vcc and vccq will be cut). > > > > In suspend/runtime_suspend, we don't really know as it depends on what > > the platform/host is capable of. If we can't do a full power-off > > (maybe just vcc can be cut), then we prefer the sleep command instead. > > I do understand that. I don't see why this needs a change in the > existing logic as Alan pointed out above. Aha. I get your point now. As stated in the commit message: Due to an earlier suspend request the eMMC may already have been properly powered-off, hence we are sometimes leaving the eMMC in its current state. However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND set we may unnecessarily restore the power to the eMMC, let's avoid this. To further clarify, at a system suspend we issue a poweroff-notify for the case above. At system resume we leave the card in powered-off state until there is I/O (when we runtime resume it). If a shutdown occurs before I/O, we would unnecessarily re-initialize the card as it's already in the correct state. Let me try to clarify the commit message a bit with this information. > > > I was hoping that patch3 should make this more clear (using an enum > > Sadly, it didn't. Using MMC_POWEROFF_SUSPEND first and then later > MMC_POWEROFF_SHUTDOWN in mmc_shutdown() is still confusing. Do you want > to return false in case none of the two PWR_CYCLE flags is set? > > > type), but I can try to add some comment(s) in the code to further > > clarify the policy. > > Please do. > > All the best, > > Wolfram > Thanks! Kind regards Uffe
Hi Ulf, > mmc_card_can_poweroff_notify() would not be consistent with all the > other mmc_can_* helpers, so I rather stay with > mmc_can_poweroff_notify(), for now. If you think a rename makes sense, > I suggest we do that as a follow up and rename all the helpers. I vageuly recall that the commit I mentioned below (renaming hw_reset to card_hw_reset) should have been a start to do exactly this, renaming more of the helpers. I drifted away. Yet, I still think this would make MMC core code a lot easier to understand. I'll work on it today, timing seems good with rc1 on the horizon... > mmc_host_can_poweroff_notify() seems fine to me! Great! > > I do understand that. I don't see why this needs a change in the > > existing logic as Alan pointed out above. > > Aha. I get your point now. As stated in the commit message: > > Due to an earlier suspend request the eMMC may already have been properly > powered-off, hence we are sometimes leaving the eMMC in its current state. > However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND > set we may unnecessarily restore the power to the eMMC, let's avoid this. Oookay, now I see what you are aiming at. It seems I got the PWR_CYCLE flags wrong? I thought MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is only a subset of MMC_CAP2_FULL_PWR_CYCLE. The former can do the power cycles only in suspend, while the latter can do them in suspend and shutdown. So, in my thinking, full power cycle might also have the eMMC powered-off during shutdown. This is wrong? > Let me try to clarify the commit message a bit with this information. Whatever is the final outcome, it needs a comment in the code, I am quite sure. Happy hacking, Wolfram
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 3424bc9e20c5..400dd0449fec 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -2014,6 +2014,18 @@ static bool mmc_can_poweroff_notify(const struct mmc_card *card) (card->ext_csd.power_off_notification == EXT_CSD_POWER_ON); } +static bool mmc_may_poweroff_notify(const struct mmc_host *host, + bool is_suspend) +{ + if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) + return true; + + if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND && is_suspend) + return true; + + return !is_suspend; +} + static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type) { unsigned int timeout = card->ext_csd.generic_cmd6_time; @@ -2124,8 +2136,7 @@ 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_IN_SUSPEND))) + mmc_may_poweroff_notify(host, is_suspend)) err = mmc_poweroff_notify(host->card, notify_type); else if (mmc_can_sleep(host->card)) err = mmc_sleep(host); @@ -2191,7 +2202,7 @@ static int mmc_shutdown(struct mmc_host *host) * before we can shutdown it properly. */ if (mmc_can_poweroff_notify(host->card) && - !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)) + !mmc_may_poweroff_notify(host, true)) err = _mmc_resume(host); if (!err)
To manage a graceful power-off of the eMMC card during platform shutdown, the preferred option is to use the poweroff-notification command. Due to an earlier suspend request the eMMC may already have been properly powered-off, hence we are sometimes leaving the eMMC in its current state. However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND set we may unnecessarily restore the power to the eMMC, let's avoid this. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/core/mmc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)