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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote: > > > > > > > > Hi Shimoda-san, > > > > > > > > CC broonie > > > > > > > > On Thu, Jun 25, 2020 at 8:31 AM Yoshihiro Shimoda > > > > <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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@xxxxxxxxxx> wrote: > > > > > > > On Mon, 22 Jun 2020 at 04:25, Yoshihiro Shimoda > > > > > > > <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx/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@xxxxxxxxxxx/T/#mde3 > 8d5866b7f3edc9a2ed105ac06b4fda4c3e99e > > > > [3] > > > https://lore.kernel.org/linux-renesas-soc/1592792699-24638-1-git-send-email-yoshihiro.shimoda.uh@xxxxxxxxxxx/T/#mf8e > 7abdf135586ba0b70a1c235410a6f94c6007d > > > > [4] > > > https://lore.kernel.org/linux-renesas-soc/CAMuHMdX93Q9WhKLqv_wNPzArbc68NcbVN8jJ9MDKxAcicpBQ5Q@xxxxxxxxxxxxxx/T/#m442 > a2ce972cfdb3ff33637c120c8d096e4d07af8 > > > > Kind regards > Uffe
