Re: [PATCH 4/5] mmc: sdhci-omap: Implement PM runtime functions

[Tony Lindgren <tony@atomide.com>]

* Ulf Hansson <ulf.hansson@linaro.org> [211012 09:07]:
> On Mon, 11 Oct 2021 at 07:23, Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Ulf Hansson <ulf.hansson@linaro.org> [211008 14:44]:
> > > On Thu, 30 Sept 2021 at 08:57, Tony Lindgren <tony@atomide.com> wrote:
> > > >
> > > > Implement PM runtime functions and enable MMC_CAP_AGGRESSIVE_PM.
> > >
> > > I suggest you split this change into two pieces. MMC_CAP_AGGRESSIVE_PM
> > > is about enabling runtime PM management for the eMMC/SD card device,
> > > which is perfectly fine to use independently of whether runtime PM is
> > > supported for the host device.
> >
> > OK
> >
> > > > @@ -1350,6 +1357,11 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > > >         if (ret)
> > > >                 goto err_cleanup_host;
> > > >
> > > > +       sdhci_omap_context_save(omap_host);
> > > > +       omap_host->context_valid = 1;
> > >
> > > Looks like you can remove this flag, it's not being used.
> >
> > Hmm I think it is needed as otherwise we end up trying to restore
> > an invalid context on probe on the first pm_runtime_get(). Do you
> > have some nicer solution for that in mind?
> 
> Right, I didn't notice that, my apologies.
> 
> In any case, an option is to bring the device into full power, without
> calling pm_runtime_resume_and_get() from ->probe(). In principle,
> running the same operations as the ->runtime_resume() callback does,
> except for restoring the context then. When this is done, the
> following calls to runtime PM should do the trick (I extended it to
> support autosuspend as well):
> 
> pm_runtime_get_noresume()
> pm_runtime_set_active()
> pm_runtime_set_autosuspend_delay()
> pm_runtime_use_autosuspend()
> pm_runtime_enable()
> 
> Note that, this means that the omaps PM domain's ->runtime_resume()
> callback doesn't get invoked when powering on the device for the first
> time. Can this be a problem?

Yeah I think that would be a problem as the register access won't work :)

I changed things around to initialize omap_host->con = -ENODEV and then
check it on resume. Not an ideal solution but avoids the extra flag.

> > > > @@ -1402,42 +1415,75 @@ static void sdhci_omap_context_restore(struct sdhci_omap_host *omap_host)
> > > >         sdhci_omap_writel(omap_host, SDHCI_OMAP_ISE, omap_host->ise);
> > > >  }
> > > >
> > > > -static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > > > +static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev)
> > > >  {
> > > >         struct sdhci_host *host = dev_get_drvdata(dev);
> > > >         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > >         struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
> > > >
> > > > -       sdhci_suspend_host(host);
> > > > -
> > >
> > > Shouldn't you call sdhci_runtime_suspend_host() somewhere here?
> >
> > I'm pretty sure I tried, but runtime resume did not seem to work after
> > doing that.. I'll take a look again.

Confusion solved for this one and it's working now.. Maybe I tried
calling sdhci_suspend_host() instead of sdhci_runtime_suspend_host()
earlier who knows.

> > > It looks a bit odd that sdhci_suspend_host() is called only when the
> > > host is runtime resumed. Perhaps you can elaborate a bit more on why
> > > this is, so I can understand better what you want to achieve here.
> >
> > I guess I'm not clear on what's left for sdhci_suspend_host() to do if
> > the host is already runtime suspended :)
> 
> I think what boils down to that is that, sdhci_suspend|resume_host()
> adds some special treatment for system wakeups (for SDIO irqs). I am
> not sure whether you may need that.

OK, will check.

> Some host drivers doesn't use sdhci_suspend|resume_host, but sticks to
> the sdhci_runtime_suspend|resume()_host() functions. Like
> sdhci-sprd.c, for example.

Hmm so is there some helper to figure out if the mmc host is active
and has a card?

Seems we could skip sdhci_suspend/resume for the inactive mmc host
instances.

Regards,

Tony