On Mon, 4 Nov 2024 at 10:22, Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote: > > When unbinding a MMC host, the card should be suspended. Otherwise, > problems may arise. E.g. the card still expects power-off notifications > but there is no host to send them anymore. Shimoda-san tried disabling > notifications only, but there were issues with his approaches [1] [2]. > > Here is my take on it, based on the review comments: > > a) 'In principle we would like to run the similar operations at "remove" > as during "system suspend"' [1] > b) 'We want to support a graceful power off sequence or the card...' [2] > > So, first, mmc_remove_card() gets improved to mark the card as "not > present" and to call the bus specific suspend() handler. > > Then, _mmc_suspend gets extended to recognize another reason of being > called, namely when a card removal happens. Building upon the now > updated mmc_remove_card(), this is the case when the card is flagged as > "not present". > > The logic of either sending a notification or sending the card to sleep > gets updated to handle this new reason. Controllers able to do full > power cycles will still do a full power cycle. Controllers which can > only do power cycles in suspend, will send the card to sleep. > > All this is for MMC. SD/SDIO are unaffected because they are not using > the 'card present' flag. > > Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > > [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/1602581312-23607-1-git-send-email-yoshihiro.shimoda.uh@xxxxxxxxxxx/ > [2] https://patchwork.kernel.org/project/linux-mmc/patch/1605005330-7178-1-git-send-email-yoshihiro.shimoda.uh@xxxxxxxxxxx/ > --- > > Lightly tested with a Renesas R-Car S4 Spider board. It bascially works > as expected. Serious testing postponed until the generic direction of > this is approved :) > > drivers/mmc/core/bus.c | 3 +++ > drivers/mmc/core/mmc.c | 29 +++++++++++++++++++++-------- > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c > index 0ddaee0eae54..52704d39c6d5 100644 > --- a/drivers/mmc/core/bus.c > +++ b/drivers/mmc/core/bus.c > @@ -403,5 +403,8 @@ void mmc_remove_card(struct mmc_card *card) > host->cqe_enabled = false; > } > > + card->state &= ~MMC_STATE_PRESENT; // TBD: mmc_card_clear_present() This is nice from a consistency point of view. Although, I don't want us to use this bit as an indication to inform the bus_ops->suspend() callback what to do. It seems prone to problems. > + host->bus_ops->suspend(host); I think this is a step in the right direction. Somehow we need to be able to call the bus_ops->suspend() before we call put_device() and before we clear the host->card pointer. However, we don't want to call bus_ops->suspend() in all cases from mmc_remove_card(), but *only* when it gets called from mmc_remove_host()->mmc_stop_host(), via the "host->bus_ops->remove(host)" callback. I am wondering whether we should just re-work/split-up the code a bit to make this work. In principle, when mmc_remove_card() is called from the path above, we should not let it call put_device(), but leave that part to the caller (mmc_stop_host()) instead. Or something along those lines. Would that work? > + > put_device(&card->dev); > } > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 6a23be214543..2bcf9ee0caa8 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -32,6 +32,12 @@ > #define MIN_CACHE_EN_TIMEOUT_MS 1600 > #define CACHE_FLUSH_TIMEOUT_MS 30000 /* 30s */ > > +enum mmc_pm_reason { > + MMC_PM_REASON_SHUTDOWN, > + MMC_PM_REASON_SUSPEND, > + MMC_PM_REASON_REMOVAL, > +}; > + > static const unsigned int tran_exp[] = { > 10000, 100000, 1000000, 10000000, > 0, 0, 0, 0 > @@ -2104,11 +2110,16 @@ static int _mmc_flush_cache(struct mmc_host *host) > return err; > } > > -static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > +static int _mmc_suspend(struct mmc_host *host, enum mmc_pm_reason reason) > { > int err = 0; > - unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT : > - EXT_CSD_POWER_OFF_LONG; > + unsigned int notify_type = reason == MMC_PM_REASON_SUSPEND ? > + EXT_CSD_POWER_OFF_SHORT : EXT_CSD_POWER_OFF_LONG; > + bool can_pwr_cycle_now = (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || > + ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND) && > + reason == MMC_PM_REASON_SUSPEND); > + > + pr_info("%s: suspend reason %d, can pwr cycle %d\n", mmc_hostname(host), reason, can_pwr_cycle_now); > > mmc_claim_host(host); > > @@ -2119,9 +2130,9 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > if (err) > goto out; > > + /* Notify if pwr_cycle is possible or power gets cut because of shutdown */ > 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))) > + (can_pwr_cycle_now || reason == MMC_PM_REASON_SHUTDOWN)) > err = mmc_poweroff_notify(host->card, notify_type); > else if (mmc_can_sleep(host->card)) > err = mmc_sleep(host); > @@ -2142,9 +2153,11 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend) > */ > static int mmc_suspend(struct mmc_host *host) > { > + enum mmc_pm_reason reason = mmc_card_present(host->card) ? > + MMC_PM_REASON_SUSPEND : MMC_PM_REASON_REMOVAL; I don't think it makes sense to differentiate between a regular "suspend" and a "host-removal". The point is, we don't know what will happen beyond the host-removal. The platform may shutdown or the host driver probes again. Let's just use the same commands as we use for suspend. > int err; > > - err = _mmc_suspend(host, true); > + err = _mmc_suspend(host, reason); > if (!err) { > pm_runtime_disable(&host->card->dev); > pm_runtime_set_suspended(&host->card->dev); > @@ -2191,7 +2204,7 @@ static int mmc_shutdown(struct mmc_host *host) > err = _mmc_resume(host); > > if (!err) > - err = _mmc_suspend(host, false); > + err = _mmc_suspend(host, MMC_PM_REASON_SHUTDOWN); > > return err; > } > @@ -2215,7 +2228,7 @@ static int mmc_runtime_suspend(struct mmc_host *host) > if (!(host->caps & MMC_CAP_AGGRESSIVE_PM)) > return 0; > > - err = _mmc_suspend(host, true); > + err = _mmc_suspend(host, MMC_PM_REASON_SUSPEND); > if (err) > pr_err("%s: error %d doing aggressive suspend\n", > mmc_hostname(host), err); > -- > 2.45.2 > Kind regards Uffe
