Hi Ulf, On Wed, Jan 22, 2025 at 10:28:09AM +0100, Wolfram Sang wrote: > Hi Ulf, > > slowly coming back to this one. > > > > + 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. > > Makes sense. We can use another bit like MMC_STATE_NEEDS_SUSPEND. That > would also... > > > > + 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. > > ... avoid this :) Refactoring every code which calls mmc_remove_card() > to handle the additional put_device() on its own seems intrusive to me. > And error prone, new users might forget to do it. And that for only one > use case where we want to do extra stuff. > > Leaving out put_device() within mmc_remove_card() only for that one case > is bad API design, of course. > > So, I envision more something along this: > > if (card->state & MMC_STATE_NEEDS_SUSPEND) > mmc_suspend() Shall I try this... > > Maybe even more generic? > > if (card->state & MMC_STATE_PREPARE_REMOVE) > bus_ops->prepare_remove() ... or even this suggestion? All the best, Wolfram > > Or am I missing something from your suggestion? > > > > + 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. > > Sadly, I think it makes sense because of our HW. We cannot control the > regulators directly. PSCI handles them. And for shutdown, it will do > "something". For normal suspend, nothing will happen because PSCI will > not be called. This is why Shimoda-san introduced > "full-pwr-cycle-in-suspend" back then. > > The differentiation is needed a little above: > > > - ((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); > > Here. Poweroff notification is only valid in the POWEROFF case for us. > > Thanks for your time, > > Wolfram >
Attachment:
signature.asc
Description: PGP signature
