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

Mon Jul 6 21:26:02 EDT 2020

Hi Ulf,

> From: Ulf Hansson, Sent: Monday, July 6, 2020 10:11 PM
> 
> On Thu, 2 Jul 2020 at 14:37, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh at renesas.com> wrote:
> >
> > 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/#m708
> 83cc5de4fa7fca50118dad743c836d5e3b451
> >
> > > 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].
> 
> This is a generic problem with FWs.
> 
> I guess one could try to update the DTB using DT overlays, in case
> some FW version changes.

It's interesting.

> >  - My environment (PSCI which is one of firmware) doesn't support
> >    any ability to sync between the firmware and OS for now [4].
> 
> Yes, I see.
> 
> It seems like Geert has tried different approaches to convince PSCI
> folkz, to find a solution for this, but it seems like none have make
> it yet.
> 
> >
> > But, what do you think?
> 
> From the mmc DT property point of view, I am fine adding a
> "full-pwr-cycle-in-suspend" - and then also update the mmc core's
> behavior to use the eMMC power-off-notify command at system suspend.
> 
> However, I don't have a strong opinion, as the current solution with
> the eMMC sleep command also seems to work.
> 
> My main concern with the current approach though, is not about wasting
> energy, but rather that we are not doing a graceful shutdown of the
> eMMC device. Instead we just cut VCCQ while the eMMC is "sleeping",
> which in worst case could lead to internal data corruptions, but also
> increased re-initialization time at system suspend.

Thank you very much for your comments! I understood your main concern.
So, I'm thinking adding a new MMC property "full-pwr-cycle-in-suspend"
is useful now. After that, user could choose a graceful shutdown instead
of "sleeping" to avoid internal data corruptions in worst case at system
suspend. I'll submit such a patch.

Best regards,
Yoshihiro Shimoda

> >
> > [2]
> >
> https://lore.kernel.org/linux-renesas-soc/1592792699-24638-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/T/#mde3
> 8d5866b7f3edc9a2ed105ac06b4fda4c3e99e
> >
> > [3]
> >
> https://lore.kernel.org/linux-renesas-soc/1592792699-24638-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/T/#mf8e
> 7abdf135586ba0b70a1c235410a6f94c6007d
> >
> > [4]
> >
> https://lore.kernel.org/linux-renesas-soc/CAMuHMdX93Q9WhKLqv_wNPzArbc68NcbVN8jJ9MDKxAcicpBQ5Q@mail.gmail.com/T/#m442
> a2ce972cfdb3ff33637c120c8d096e4d07af8
> >
> 
> Kind regards
> Uffe