Re: [PATCH v7] mmc: sdhci-of-dwcmshc: Add runtime PM operations

[Ulf Hansson]

[...]

> > > >>>>
> > > >>>> 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