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

[Geert Uytterhoeven]

Hi Shimoda-san,

CC broonie

On Thu, Jun 25, 2020 at 8:31 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh at 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 at linaro.org> wrote:
> > > On Mon, 22 Jun 2020 at 04:25, Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh at 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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds