Re: [PATCH v2 8/8] mmc: sunxi: Add runtime_pm support

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2533 bytes --]

Hi,

On Thu, Mar 15, 2018 at 11:24:36AM +0100, Ulf Hansson wrote:
> >> > +static int sunxi_mmc_runtime_resume(struct device *dev)
> >> > +{
> >> > +       struct mmc_host *mmc = dev_get_drvdata(dev);
> >> > +       struct sunxi_mmc_host *host = mmc_priv(mmc);
> >> > +       int ret;
> >> > +
> >> > +       ret = sunxi_mmc_enable(host);
> >> > +       if (ret)
> >> > +               return ret;
> >> > +
> >> > +       sunxi_mmc_power_up(mmc, &mmc->ios);
> >>
> >> Instead of doing power up, you may need restore some ios settings,
> >> such as the clock rate for example.
> >>
> >> You may also need to restore some registers in sunxi device, in case
> >> it's possible that the controller loses context at runtime suspend.
> >
> > The thing I was precisely trying to avoid was this :)
> >
> > I could save and restore registers when the MMC controller is put into
> > suspend, but that's pretty much exactly the same sequence than what is
> > done in the MMC_POWER_UP sequence in .set_ios.
> 
> Well, there may be some overlap.
> 
> >
> > So it just felt cleaner to just do the power_up sequence at resume
> > time. It avoids having to maintain the list of registers to save and
> > restore, and it involves less code overall.
> 
> I understand.
> 
> >
> > It suprised me a bit that the core will not call .set_ios with
> > MMC_POWER_UP at resume by itself, but I guess there's a good reason
> > for that :)
> 
> It does that when it runtime PM manages the mmc/sd/sdio card, don't
> confuse that with the mmc host. That's because it needs to follow the
> (e)MMC/SD/SDIO spec, which don't allows you to just cut the power to
> card without first informing (sending commands) the card about it.

Ok. So this might sound a bit trivial, but I'm confused now about what
set_ios is supposed to be doing.

I thought this was about the MMC controller itself, but from what
you're saying, it seems that it is used for both the MMC controller
and the MMC card itself, with the powermode being about the MMC card,
and the rest about the MMC controller.

I guess we're mixing the two then, especially the power off part that
will also reset the controller, or the controller that is initialised
only at power on. Other MMC controller drivers I could find were doing
the MMC controller setup unconditionally, so I guess we have room for
improvements here.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel