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

[Yoshihiro Shimoda]

Hi Ulf,

> From: Ulf Hansson, Sent: Friday, June 26, 2020 8:03 PM
> 
> + Rob
> 
> On Thu, 25 Jun 2020 at 11:26, Geert Uytterhoeven <geert at linux-m68k.org> wrote:
> >
> > 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.

As other email thread of this trial as v4, I could not get approval [1].

[1]
https://lore.kernel.org/linux-renesas-soc/CAMuHMdX93Q9WhKLqv_wNPzArbc68NcbVN8jJ9MDKxAcicpBQ5Q@mail.gmail.com/T/#m70883cc5de4fa7fca50118dad743c836d5e3b451

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

My v4 patch was using a regulator property. But since I could not get
approval, we could not use this way, I think.

> If that doesn't make sense, I am also open to add a
> new MMC property, "full-pwr-cycle-in-suspend".

I thought this way was better. However, I'm wondering if adding such a new MMC
property to issue Power Off Notification in mmc_suspend() is really better
than the current implementation. This is because we don't have any completely
solutions now:
 - Depend on board configuration (The board doesn't have "full-pwr-cycle").
 - Depend on firmware on board [2]. So, even if adding a new MMC property,
   it cannot sync the firmware condition. In fact, this is possible to
   cause regression in power consumption point of view [3].
 - My environment (PSCI which is one of firmware) doesn't support
   any ability to sync between the firmware and OS for now [4].

But, what do you think?

[2]
https://lore.kernel.org/linux-renesas-soc/1592792699-24638-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/T/#mde38d5866b7f3edc9a2ed105ac06b4fda4c3e99e

[3]
https://lore.kernel.org/linux-renesas-soc/1592792699-24638-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/T/#mf8e7abdf135586ba0b70a1c235410a6f94c6007d

[4]
https://lore.kernel.org/linux-renesas-soc/CAMuHMdX93Q9WhKLqv_wNPzArbc68NcbVN8jJ9MDKxAcicpBQ5Q@mail.gmail.com/T/#m442a2ce972cfdb3ff33637c120c8d096e4d07af8

Best regards,
Yoshihiro Shimoda

> Kind regards
> Uffe