Re: [PATCH v3 2/2] mmc: core: Call mmc_poweroff_nofity() if pm_suspend_via_firmware()

[Ulf Hansson <ulf.hansson@linaro.org>]

+ Rob

On Thu, 25 Jun 2020 at 11:26, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Shimoda-san,
>
> CC broonie
>
> On Thu, Jun 25, 2020 at 8:31 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Geert Uytterhoeven, Sent: Wednesday, June 24, 2020 8:13 PM
> > > On Wed, Jun 24, 2020 at 12:06 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Mon, 22 Jun 2020 at 04:25, Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > If pm_suspend_via_firmware() returns true, the system will be able
> > > > > to cut both vcc and vccq in the suspend. So, call
> > > > > mmc_poweroff_nofity() if pm_suspend_via_firmware() returns true.
> > > > >
> > > > > Note that we should not update the MMC_CAP2_FULL_PWR_CYCLE caps
> > > > > because the mmc_select_voltage() checks the caps when attaches
> > > > > a mmc/sd.
> > >
> > > > > --- a/drivers/mmc/core/mmc.c
> > > > > +++ b/drivers/mmc/core/mmc.c
> > > > > @@ -2038,7 +2039,8 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
> > > > >                 goto out;
> > > > >
> > > > >         if (mmc_can_poweroff_notify(host->card) &&
> > > > > -               ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend))
> > > > > +           ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
> > > > > +            pm_suspend_via_firmware()))
> > > >
> > > > Sorry, but this doesn't work.
> > > >
> > > > Even if PSCI is a generic FW interface, it doesn't mean that all PSCI
> > > > implementations will cut the vcc and vccq for the MMC card at system
> > > > suspend.
> > >
> > > Indeed, there's nothing guaranteed here.  Nor documented how it should
> > > behave.  Basically the firmware is free to power off the SoC. Or not do that.
> > > "If firmware is involved, all odds are off".
> >
> > I thought we could be guaranteed. But, I understood we could not be guaranteed...
> >
> > > > Instead, you need to decide this based on some specific DT property.
> > > > Perhaps in conjunction with using pm_suspend_via_firmware().
> > >
> > > Last time I was involved in a discussion about this, the PSCI people
> > > didn't want to add any properties describing particular PSCI behavior...
> > > "If firmware is involved, all odds are off".
> > >
> > > So the only safe thing to do is to expect the worst, and prepare for it...
> >
> > A headache point is an eMMC device consumes much power if that the system
> > doesn't cut the vcc and vccq and doesn’t enter the sleep mode.
> > In other words, in power consumption point of view, this patch will
> > cause a regression in such a case...
>
> Indeed.
>
> > By the way, about adding specific DT property, the regulator can have
> > regulator-off-in-suspend property in regulator-state-mem subnode.
> > For now, we doesn't seem to get the property from a regulator consumer though.
> > So, I'll try to add an API of regulator for it.
>
> Oh right, the eMMC is described in DT as being connected to two
> regulators.
> Note that the semantics of regulator-off-in-suspend are that the
> regulator should be disabled (by the regulator core) during suspend, not
> that the regulator is disabled during suspend by a third party.
> No idea if that will work with a fixed-regulator without GPIO control,
> but of course you can try.

For mmc we already have a DT property, "full-pwr-cycle". Which tells
whether the host is able to completely power-cycle the card (some
host's manage power internally).

However, maybe the proper thing here would be to add a property of
regulator instead. If that doesn't make sense, I am also open to add a
new MMC property, "full-pwr-cycle-in-suspend".

Kind regards
Uffe