On Wed, 15 Apr 2020 at 01:16, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Ulf Hansson (2020-03-20 03:22:01) > > On Thu, 19 Mar 2020 at 18:42, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > > > Quoting Ulf Hansson (2020-03-06 02:07:41) > > > > On Wed, 4 Mar 2020 at 17:46, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > > > > > > > Quoting Ulf Hansson (2020-03-04 07:34:29) > > > > > > On Thu, 20 Feb 2020 at 07:45, Shaik Sajida Bhanu <sbhanu@xxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > The existing suspend/resume callbacks of sdhci-msm driver are just > > > > > > > gating/un-gating the clocks. During suspend cycle more can be done > > > > > > > like disabling controller, disabling card detection, enabling wake-up events. > > > > > > > > > > > > > > So updating the system pm callbacks for performing these extra > > > > > > > actions besides controlling the clocks. > > > > > > > > > > > > > > Signed-off-by: Shaik Sajida Bhanu <sbhanu@xxxxxxxxxxxxxx> > > > > > > > Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > > > > > > > --- > > > [...] > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + ret = pm_runtime_force_suspend(dev); > > > > > > > > > > > > It looks to me that perhaps you could make use of solely > > > > > > pm_runtime_force_suspend(), then just skip calling > > > > > > sdhci_suspend|resume_host() altogether. Do you think that could work? > > > > > > > > > > Does that do all the things the commit text mentions is desired for > > > > > system suspend? > > > > > > > > No. :-) > > > > > > > > But why is system wakeup needed for an eMMC card? > > > > > > > > > > I don't know if system wakeup is needed for an eMMC card. Probably only > > > if you plug in a card and some daemon wants to wake up and probe the > > > card for auto-play or something like that? Seems possible so might as > > > well expose the CD gpio as a wakeup in that case and let userspace > > > decide if it wants to do that. > > > > Right, card detect IRQs could be useful for system wakeups. > > > > I assume you are using a GPIO IRQ for that, which is easily managed, > > as the runtime PM status of the mmc controller is irrelevant when > > configuring the GPIO IRQ as wakeup. > > > > We even have a helper for doing this, mmc_gpio_set_cd_wake(). > > Right. Maybe mmc_gpio_set_cd_wake() needs to be called from somewhere in > the sdhci core? Yes, that seems reasonable. > > > > > > > > > Is runtime suspended state the same as system suspended state here > > > though? The commit text seems to imply that only clks are disabled when > > > it's desirable to disable the entire controller. I'm still fuzzy on how > > > runtime PM and system PM interact because it seems to have changed since > > > I looked last a few years ago. If the driver can stay in a runtime > > > suspended state across system suspend then I'm all for it. That would > > > save time for system PM transitions. > > > > In most cases this should be possible. And so far, for this case, I > > haven't found a good reason to why it shouldn't work. > > > > Although, perhaps we need to improve some of the sdhci's library > > functions for PM, to better support this. > > > > So does that mean it's all just working then? Nothing to do here except > make wakeup irqs for CD work? Well, if it "works " or not, I am not really sure. My point is, I think most of the things that need to be managed at system suspend/resume are the same things that need to be managed during runtime suspend/resume (except wakeups). So, rather than implementing a whole bunch of system suspend/resume specific things, why not make use of the runtime suspend/resume callbacks instead. Kind regards Uffe
