[...] > > > >>>> > > > >>>> Since you are only using the runtime PM callbacks to turn off the card > > > >>>> clock via SDHCI_CLOCK_CONTROL, pm_runtime_force_suspend() and > > > >>>> pm_runtime_force_resume() are not needed at all. > > > >>> > > > >>> Right, it can be done without these too. > > > >>> > > > >>>> > > > >>>> sdhci_suspend_host() does not care if SDHCI_CLOCK_CARD_EN is on or > > off. > > > >>>> (And you are disabling pltfm_host->clk and priv->bus_clk, so > > presumably > > > >>>> the result is no clock either way) > > > >>>> > > > >>>> sdhci_resume_host() does not restore state unless > > > >>>> SDHCI_QUIRK2_HOST_OFF_CARD_ON is used, it just resets, so the > > internal clock > > > >>>> SDHCI_CLOCK_INT_EN is off which is consistent with either runtime > > suspended > > > >>>> or runtime resumed. > > > >>> > > > >>> Even if this may work, to me, it doesn't look like good practice for > > > >>> how to use runtime PM in combination with system wide > > suspend/resume. > > > >>> > > > >>> The point is, sdhci_suspend|resume_host() may end up reading/writing > > > >>> to sdhci registers - and we should *not* allow that (because it may > > > >>> not always work), unless the sdhci controller has been runtime resumed > > > >>> first, right? > > > >> > > > >> I am OK with drivers that just want to use runtime PM to turn off a > > > >> functional clock. sdhci-tegra.c is also doing that although using the > > > >> clock framework. > > > > > > > > Yes, I agree. At least this works for SoC specific drivers. > > > > > > > >> > > > >> Certainly that approach assumes that the host controller's power state > > > >> is not changed due to runtime PM. > > > >> > > > >> To ensure that the host controller is runtime resumed before calling > > > >> sdhci_suspend_host(), we can just call pm_runtime_resume() I think. > > > > > > > > Yes, that was kind of what I proposed in the other thread as option 1) > > > > (except for the replacement of pm_runtime_force_suspend|resume). > > > > > > > > Although, to be clear I would probably use pm_runtime_get_sync() > > > > instead, to make sure the usage count is incremented too. > > > > > > In that case, a matching pm_runtime_put() is needed also at the > > > end of the resume callback. > > > > Yes, of course. Or depending if we are using the force_suspend|resume > > helper, a pm_runtime_put_noidle is sufficient after > > pm_runtime_force_suspend() has been called. > > Thanks Ulf/Adrian! Plan to upload v8 with the following changes: > - Remove pm_runtime_force_suspend/resume() from dwcmshc_suspend()/dwcmshc_resume() (Adrian's comment). > - Add comments for dwcmshc_resume()/dwcmshc_suspend(); > (According to Andrian's comment). > - Add pm_runtime_get_sync()/pm_runtime_put() in dwcmshc_suspend(), which is Ulf option-1. Option-2 seems more efficient, but it involves more changes and I couldn't test the impact on other SoC. Maybe for future enhancement? That works for me! Kind regards Uffe
