[PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Yoshihiro Shimoda]

The commit 432356793415 ("mmc: core: Enable power_off_notify for
eMMC shutdown sequence") enabled the power off notification
even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
not set. However, the mmc core lacks to issue the power off
notificaiton when Suspend-to-{RAM,Disk} happens on the system.

So, add Suspend-to-RAM support at first because this is easy to
check by using pm_suspend_target_state condition on _mmc_suspend().

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/core/mmc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 4203303..4a23f83 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
+#include <linux/suspend.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/mmc/host.h>
@@ -2027,6 +2028,12 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 	int err = 0;
 	unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
 					EXT_CSD_POWER_OFF_LONG;
+	bool s2ram = false;
+
+#ifdef CONFIG_PM_SLEEP
+	if (pm_suspend_target_state == PM_SUSPEND_MEM)
+		s2ram = true;
+#endif
 
 	mmc_claim_host(host);
 
@@ -2038,7 +2045,7 @@ 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 || s2ram))
 		err = mmc_poweroff_notify(host->card, notify_type);
 	else if (mmc_can_sleep(host->card))
 		err = mmc_sleep(host);
-- 
2.7.4

RE: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Yoshihiro Shimoda]

Hi again,

> From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> 
> The commit 432356793415 ("mmc: core: Enable power_off_notify for
> eMMC shutdown sequence") enabled the power off notification
> even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> not set. However, the mmc core lacks to issue the power off
> notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> 
> So, add Suspend-to-RAM support at first because this is easy to
> check by using pm_suspend_target_state condition on _mmc_suspend().
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I'd like to add more detail why this patch is needed.
I think we should think some events (which are Shutdown, Suspend-to-idle,
Suspend-to-RAM) for the Power off Notification control.
I described these events like below.

Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
Assumption of the eMMC : in POWERED_ON

1) Event  : Shutdown
- power   : going to VCC=OFF & VCCQ=OFF
- ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
- current : POWER_OFF_LONG --> Perfect
- Remarks : the commit 432356793415

2) Event  : Suspend-to-Idle
- power   : Keep VCC=ON & VCCQ=ON
- ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
- current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
- Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).

3) Event  : Suspend-to-RAM
- power   : going to VCC=OFF & VCCQ=OFF
- ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT (because the same as the "Shutdown" event)
- current : issue MMC_SLEEP_AWAKE --> NG
- Remarks : So, I tried to fix this by the patch.

In addition, we should also think about the event of unbind.

4) Event  : Unbind
- power   : Keep VCC=ON & VCCQ=ON
- ideal   : NO_POWER_NOTIFICATION because user is possible to turn the power off
- current : Keep POWERED_ON --> NG
- Remarks : But, I didn't try to fix this yet.

Best regards,
Yoshihiro Shimoda

Re: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Ulf Hansson]

On Thu, 4 Jun 2020 at 14:17, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
> Hi again,
>
> > From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> >
> > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > eMMC shutdown sequence") enabled the power off notification
> > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > not set. However, the mmc core lacks to issue the power off
> > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> >
> > So, add Suspend-to-RAM support at first because this is easy to
> > check by using pm_suspend_target_state condition on _mmc_suspend().
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>
> I'd like to add more detail why this patch is needed.
> I think we should think some events (which are Shutdown, Suspend-to-idle,
> Suspend-to-RAM) for the Power off Notification control.
> I described these events like below.
>
> Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
> Assumption of the eMMC : in POWERED_ON
>
> 1) Event  : Shutdown
> - power   : going to VCC=OFF & VCCQ=OFF
> - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
> - current : POWER_OFF_LONG --> Perfect
> - Remarks : the commit 432356793415
>
> 2) Event  : Suspend-to-Idle
> - power   : Keep VCC=ON & VCCQ=ON
> - ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
> - current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
> - Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).

As a matter of fact, VCCQ *must* remain on in sleep state, while VCC
can be powered off.

>
> 3) Event  : Suspend-to-RAM
> - power   : going to VCC=OFF & VCCQ=OFF

I don't understand why you think S2R should be treated differently
from S2I? At least from the MMC subsystem point of view, there is no
difference. No?

> - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT (because the same as the "Shutdown" event)
> - current : issue MMC_SLEEP_AWAKE --> NG
> - Remarks : So, I tried to fix this by the patch.
>
> In addition, we should also think about the event of unbind.

Yes, very good point.

>
> 4) Event  : Unbind
> - power   : Keep VCC=ON & VCCQ=ON
> - ideal   : NO_POWER_NOTIFICATION because user is possible to turn the power off
> - current : Keep POWERED_ON --> NG
> - Remarks : But, I didn't try to fix this yet.

I don't quite understand why we should keep VCC and VCCQ on?

In principle I think we should treat "unbind" in the similar way as we
treat S2R/S2I. Which means sending power-off-notification if the host
supports MMC_CAP2_FULL_PWR_CYCLE, otherwise we should send sleep.

Kind regards
Uffe

RE: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Yoshihiro Shimoda]

> From: Ulf Hansson, Sent: Monday, June 8, 2020 5:14 PM
> On Thu, 4 Jun 2020 at 14:17, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> > Hi again,
> >
> > > From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> > >
> > > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > > eMMC shutdown sequence") enabled the power off notification
> > > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > > not set. However, the mmc core lacks to issue the power off
> > > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> > >
> > > So, add Suspend-to-RAM support at first because this is easy to
> > > check by using pm_suspend_target_state condition on _mmc_suspend().
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > I'd like to add more detail why this patch is needed.
> > I think we should think some events (which are Shutdown, Suspend-to-idle,
> > Suspend-to-RAM) for the Power off Notification control.
> > I described these events like below.
> >
> > Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
> > Assumption of the eMMC : in POWERED_ON
> >
> > 1) Event  : Shutdown
> > - power   : going to VCC=OFF & VCCQ=OFF
> > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
> > - current : POWER_OFF_LONG --> Perfect
> > - Remarks : the commit 432356793415
> >
> > 2) Event  : Suspend-to-Idle
> > - power   : Keep VCC=ON & VCCQ=ON
> > - ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
> > - current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
> > - Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).
> 
> As a matter of fact, VCCQ *must* remain on in sleep state, while VCC
> can be powered off.

I got it.

> >
> > 3) Event  : Suspend-to-RAM
> > - power   : going to VCC=OFF & VCCQ=OFF
> 
> I don't understand why you think S2R should be treated differently
> from S2I? At least from the MMC subsystem point of view, there is no
> difference. No?

On my environment, VCC & VCCQ condition differs like below.
 S2I: VCC=ON & VCCQ=ON
 S2R: VCC=OFF & VCCQ=OFF

So, I think the MMC subsystem should not enter sleep mode
on such environment. If this is board-specific, I'm thinking
I should add a new flag to fix the issue which is entering
sleep mode even if VCCQ=OFF.

> > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT (because the same as the "Shutdown" event)
> > - current : issue MMC_SLEEP_AWAKE --> NG
> > - Remarks : So, I tried to fix this by the patch.
> >
> > In addition, we should also think about the event of unbind.
> 
> Yes, very good point.
> 
> >
> > 4) Event  : Unbind
> > - power   : Keep VCC=ON & VCCQ=ON
> > - ideal   : NO_POWER_NOTIFICATION because user is possible to turn the power off
> > - current : Keep POWERED_ON --> NG
> > - Remarks : But, I didn't try to fix this yet.
> 
> I don't quite understand why we should keep VCC and VCCQ on?

Oops. I should have described a use case. I thought one of use cases was
using mmc_test driver. So, I thought we should keep VCC and VCCQ on to
use mmc_test driver.

> In principle I think we should treat "unbind" in the similar way as we
> treat S2R/S2I. Which means sending power-off-notification if the host
> supports MMC_CAP2_FULL_PWR_CYCLE, otherwise we should send sleep.

If we didn't take care of mmc_test driver after unbind, I think so.

Best regards,
Yoshihiro Shimoda

> Kind regards
> Uffe

Re: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Ulf Hansson]

On Mon, 8 Jun 2020 at 12:39, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
> > From: Ulf Hansson, Sent: Monday, June 8, 2020 5:14 PM
> > On Thu, 4 Jun 2020 at 14:17, Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > >
> > > Hi again,
> > >
> > > > From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> > > >
> > > > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > > > eMMC shutdown sequence") enabled the power off notification
> > > > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > > > not set. However, the mmc core lacks to issue the power off
> > > > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> > > >
> > > > So, add Suspend-to-RAM support at first because this is easy to
> > > > check by using pm_suspend_target_state condition on _mmc_suspend().
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > I'd like to add more detail why this patch is needed.
> > > I think we should think some events (which are Shutdown, Suspend-to-idle,
> > > Suspend-to-RAM) for the Power off Notification control.
> > > I described these events like below.
> > >
> > > Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
> > > Assumption of the eMMC : in POWERED_ON
> > >
> > > 1) Event  : Shutdown
> > > - power   : going to VCC=OFF & VCCQ=OFF
> > > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
> > > - current : POWER_OFF_LONG --> Perfect
> > > - Remarks : the commit 432356793415
> > >
> > > 2) Event  : Suspend-to-Idle
> > > - power   : Keep VCC=ON & VCCQ=ON
> > > - ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
> > > - current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
> > > - Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).
> >
> > As a matter of fact, VCCQ *must* remain on in sleep state, while VCC
> > can be powered off.
>
> I got it.
>
> > >
> > > 3) Event  : Suspend-to-RAM
> > > - power   : going to VCC=OFF & VCCQ=OFF
> >
> > I don't understand why you think S2R should be treated differently
> > from S2I? At least from the MMC subsystem point of view, there is no
> > difference. No?
>
> On my environment, VCC & VCCQ condition differs like below.
>  S2I: VCC=ON & VCCQ=ON
>  S2R: VCC=OFF & VCCQ=OFF

Can you explain why it differs? Who is managing the regulators and who
decides to turn them off?

Perhaps this is a regulator-enable usage count problem?

>
> So, I think the MMC subsystem should not enter sleep mode
> on such environment. If this is board-specific, I'm thinking
> I should add a new flag to fix the issue which is entering
> sleep mode even if VCCQ=OFF.
>
> > > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT (because the same as the "Shutdown" event)
> > > - current : issue MMC_SLEEP_AWAKE --> NG
> > > - Remarks : So, I tried to fix this by the patch.
> > >
> > > In addition, we should also think about the event of unbind.
> >
> > Yes, very good point.
> >
> > >
> > > 4) Event  : Unbind
> > > - power   : Keep VCC=ON & VCCQ=ON
> > > - ideal   : NO_POWER_NOTIFICATION because user is possible to turn the power off
> > > - current : Keep POWERED_ON --> NG
> > > - Remarks : But, I didn't try to fix this yet.
> >
> > I don't quite understand why we should keep VCC and VCCQ on?
>
> Oops. I should have described a use case. I thought one of use cases was
> using mmc_test driver. So, I thought we should keep VCC and VCCQ on to
> use mmc_test driver.

Okay, let me think about this.

Actually, we can also unbind the mmc host. And if re-binding again,
that should still work.

>
> > In principle I think we should treat "unbind" in the similar way as we
> > treat S2R/S2I. Which means sending power-off-notification if the host
> > supports MMC_CAP2_FULL_PWR_CYCLE, otherwise we should send sleep.
>
> If we didn't take care of mmc_test driver after unbind, I think so.

Kind regards
Uffe

Re: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Jun 8, 2020 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Mon, 8 Jun 2020 at 12:39, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Ulf Hansson, Sent: Monday, June 8, 2020 5:14 PM
> > > On Thu, 4 Jun 2020 at 14:17, Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> > > > >
> > > > > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > > > > eMMC shutdown sequence") enabled the power off notification
> > > > > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > > > > not set. However, the mmc core lacks to issue the power off
> > > > > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> > > > >
> > > > > So, add Suspend-to-RAM support at first because this is easy to
> > > > > check by using pm_suspend_target_state condition on _mmc_suspend().
> > > > >
> > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > >
> > > > I'd like to add more detail why this patch is needed.
> > > > I think we should think some events (which are Shutdown, Suspend-to-idle,
> > > > Suspend-to-RAM) for the Power off Notification control.
> > > > I described these events like below.
> > > >
> > > > Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
> > > > Assumption of the eMMC : in POWERED_ON
> > > >
> > > > 1) Event  : Shutdown
> > > > - power   : going to VCC=OFF & VCCQ=OFF
> > > > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
> > > > - current : POWER_OFF_LONG --> Perfect
> > > > - Remarks : the commit 432356793415
> > > >
> > > > 2) Event  : Suspend-to-Idle
> > > > - power   : Keep VCC=ON & VCCQ=ON
> > > > - ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
> > > > - current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
> > > > - Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).
> > >
> > > As a matter of fact, VCCQ *must* remain on in sleep state, while VCC
> > > can be powered off.
> >
> > I got it.
> >
> > > >
> > > > 3) Event  : Suspend-to-RAM
> > > > - power   : going to VCC=OFF & VCCQ=OFF
> > >
> > > I don't understand why you think S2R should be treated differently
> > > from S2I? At least from the MMC subsystem point of view, there is no
> > > difference. No?
> >
> > On my environment, VCC & VCCQ condition differs like below.
> >  S2I: VCC=ON & VCCQ=ON
> >  S2R: VCC=OFF & VCCQ=OFF
>
> Can you explain why it differs? Who is managing the regulators and who
> decides to turn them off?

The firmware does, through PSCI system suspend.
And what it does exactly is not standardized.
Perhaps we do need an "arm,psci-system-suspend-is-power-down"[1]
DT property?

> Perhaps this is a regulator-enable usage count problem?

Unfortunately not. Else we could fix it :-)

[1] "[PATCH/RFC 4/6] drivers: firmware: psci: Fix non-PMIC wake-up if
SYSTEM_SUSPEND cuts power"
      https://lore.kernel.org/linux-arm-kernel/1487622809-25127-5-git-send-email-geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

Re: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Ulf Hansson]

On Mon, 8 Jun 2020 at 14:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Mon, Jun 8, 2020 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Mon, 8 Jun 2020 at 12:39, Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > From: Ulf Hansson, Sent: Monday, June 8, 2020 5:14 PM
> > > > On Thu, 4 Jun 2020 at 14:17, Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> > > > > >
> > > > > > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > > > > > eMMC shutdown sequence") enabled the power off notification
> > > > > > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > > > > > not set. However, the mmc core lacks to issue the power off
> > > > > > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> > > > > >
> > > > > > So, add Suspend-to-RAM support at first because this is easy to
> > > > > > check by using pm_suspend_target_state condition on _mmc_suspend().
> > > > > >
> > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > >
> > > > > I'd like to add more detail why this patch is needed.
> > > > > I think we should think some events (which are Shutdown, Suspend-to-idle,
> > > > > Suspend-to-RAM) for the Power off Notification control.
> > > > > I described these events like below.
> > > > >
> > > > > Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
> > > > > Assumption of the eMMC : in POWERED_ON
> > > > >
> > > > > 1) Event  : Shutdown
> > > > > - power   : going to VCC=OFF & VCCQ=OFF
> > > > > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
> > > > > - current : POWER_OFF_LONG --> Perfect
> > > > > - Remarks : the commit 432356793415
> > > > >
> > > > > 2) Event  : Suspend-to-Idle
> > > > > - power   : Keep VCC=ON & VCCQ=ON
> > > > > - ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
> > > > > - current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
> > > > > - Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).
> > > >
> > > > As a matter of fact, VCCQ *must* remain on in sleep state, while VCC
> > > > can be powered off.
> > >
> > > I got it.
> > >
> > > > >
> > > > > 3) Event  : Suspend-to-RAM
> > > > > - power   : going to VCC=OFF & VCCQ=OFF
> > > >
> > > > I don't understand why you think S2R should be treated differently
> > > > from S2I? At least from the MMC subsystem point of view, there is no
> > > > difference. No?
> > >
> > > On my environment, VCC & VCCQ condition differs like below.
> > >  S2I: VCC=ON & VCCQ=ON
> > >  S2R: VCC=OFF & VCCQ=OFF
> >
> > Can you explain why it differs? Who is managing the regulators and who
> > decides to turn them off?
>
> The firmware does, through PSCI system suspend.
> And what it does exactly is not standardized.

This sounds really weird. Especially, to let PSCI handle the VCC
regulator seems wrong, while PSCI is about power for CPUs and CPU
clusters (and corresponding power rails).

Oh well, nevermind.

> Perhaps we do need an "arm,psci-system-suspend-is-power-down"[1]
> DT property?

Hmm.

I wouldn't limit this to PSCI, but rather see this as a generic FW issue.

In principle, it sounds to me, like we need to dynamically allow the
mmc host to update MMC_CAP2_FULL_PWR_CYCLE, depending on what system
suspend mode we are about to enter. Or something along those lines.

>
> > Perhaps this is a regulator-enable usage count problem?
>
> Unfortunately not. Else we could fix it :-)

I see.

[...]

Kind regards
Uffe

RE: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Yoshihiro Shimoda]

Hi Ulf,

> From: Ulf Hansson, Sent: Monday, June 8, 2020 11:51 PM
> 
> On Mon, 8 Jun 2020 at 14:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Ulf,
> >
> > On Mon, Jun 8, 2020 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Mon, 8 Jun 2020 at 12:39, Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > From: Ulf Hansson, Sent: Monday, June 8, 2020 5:14 PM
> > > > > On Thu, 4 Jun 2020 at 14:17, Yoshihiro Shimoda
> > > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > > From: Yoshihiro Shimoda, Sent: Tuesday, May 19, 2020 8:33 PM
> > > > > > >
> > > > > > > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > > > > > > eMMC shutdown sequence") enabled the power off notification
> > > > > > > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > > > > > > not set. However, the mmc core lacks to issue the power off
> > > > > > > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> > > > > > >
> > > > > > > So, add Suspend-to-RAM support at first because this is easy to
> > > > > > > check by using pm_suspend_target_state condition on _mmc_suspend().
> > > > > > >
> > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > > >
> > > > > > I'd like to add more detail why this patch is needed.
> > > > > > I think we should think some events (which are Shutdown, Suspend-to-idle,
> > > > > > Suspend-to-RAM) for the Power off Notification control.
> > > > > > I described these events like below.
> > > > > >
> > > > > > Assumption of the host : MMC_CAP2_FULL_PWR_CYCLE=false
> > > > > > Assumption of the eMMC : in POWERED_ON
> > > > > >
> > > > > > 1) Event  : Shutdown
> > > > > > - power   : going to VCC=OFF & VCCQ=OFF
> > > > > > - ideal   : Either POWER_OFF_LONG or POWER_OFF_SHORT
> > > > > > - current : POWER_OFF_LONG --> Perfect
> > > > > > - Remarks : the commit 432356793415
> > > > > >
> > > > > > 2) Event  : Suspend-to-Idle
> > > > > > - power   : Keep VCC=ON & VCCQ=ON
> > > > > > - ideal   : issue MMC_SLEEP_AWAKE and keep the power (because the host could not change VCC=OFF)
> > > > > > - current : issue MMC_SLEEP_AWAKE and keep the power --> Perfect
> > > > > > - Remarks : IIUC, even if the eMMC is in POWERED_ON, a host can issue CMD5 (sleep).
> > > > >
> > > > > As a matter of fact, VCCQ *must* remain on in sleep state, while VCC
> > > > > can be powered off.
> > > >
> > > > I got it.
> > > >
> > > > > >
> > > > > > 3) Event  : Suspend-to-RAM
> > > > > > - power   : going to VCC=OFF & VCCQ=OFF
> > > > >
> > > > > I don't understand why you think S2R should be treated differently
> > > > > from S2I? At least from the MMC subsystem point of view, there is no
> > > > > difference. No?
> > > >
> > > > On my environment, VCC & VCCQ condition differs like below.
> > > >  S2I: VCC=ON & VCCQ=ON
> > > >  S2R: VCC=OFF & VCCQ=OFF
> > >
> > > Can you explain why it differs? Who is managing the regulators and who
> > > decides to turn them off?
> >
> > The firmware does, through PSCI system suspend.
> > And what it does exactly is not standardized.
> 
> This sounds really weird. Especially, to let PSCI handle the VCC
> regulator seems wrong, while PSCI is about power for CPUs and CPU
> clusters (and corresponding power rails).
> 
> Oh well, nevermind.
> 
> > Perhaps we do need an "arm,psci-system-suspend-is-power-down"[1]
> > DT property?
> 
> Hmm.
> 
> I wouldn't limit this to PSCI, but rather see this as a generic FW issue.
> 
> In principle, it sounds to me, like we need to dynamically allow the
> mmc host to update MMC_CAP2_FULL_PWR_CYCLE, depending on what system
> suspend mode we are about to enter. Or something along those lines.

I see.
However, I have no idea how to inform to a host about the FW will turn
the power off in suspend mode for now...

By the way, this is a workaround though, to avoid the issue (entering
sleep mode and power off), could we add a new property to a mmc host?

Best regards,
Yoshihiro Shimoda

> >
> > > Perhaps this is a regulator-enable usage count problem?
> >
> > Unfortunately not. Else we could fix it :-)
> 
> I see.
> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Ulf Hansson]

On Tue, 19 May 2020 at 13:33, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
> The commit 432356793415 ("mmc: core: Enable power_off_notify for
> eMMC shutdown sequence") enabled the power off notification
> even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> not set. However, the mmc core lacks to issue the power off
> notificaiton when Suspend-to-{RAM,Disk} happens on the system.

This isn't an entirely correct description, I think.

If the host supports MMC_CAP2_FULL_PWR_CYCLE (both vmmc and vqmmc can
be powered on/off), we use power-off-notification during system
suspend, in case the eMMC card also supports it. Otherwise we send the
sleep command.

This behaviour was decided on purpose and it's mainly because without
MMC_CAP2_FULL_PWR_CYCLE, we assume that vqmmc remains always-on. In
this case, it simply seemed better to use the sleep command, rather
than the power-off-notification, as we aren't really going to do a
full power off anyway.

Kind regards
Uffe

>
> So, add Suspend-to-RAM support at first because this is easy to
> check by using pm_suspend_target_state condition on _mmc_suspend().
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/mmc/core/mmc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 4203303..4a23f83 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> +#include <linux/suspend.h>
>  #include <linux/pm_runtime.h>
>
>  #include <linux/mmc/host.h>
> @@ -2027,6 +2028,12 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>         int err = 0;
>         unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
>                                         EXT_CSD_POWER_OFF_LONG;
> +       bool s2ram = false;
> +
> +#ifdef CONFIG_PM_SLEEP
> +       if (pm_suspend_target_state == PM_SUSPEND_MEM)
> +               s2ram = true;
> +#endif
>
>         mmc_claim_host(host);
>
> @@ -2038,7 +2045,7 @@ 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 || s2ram))
>                 err = mmc_poweroff_notify(host->card, notify_type);
>         else if (mmc_can_sleep(host->card))
>                 err = mmc_sleep(host);
> --
> 2.7.4
>

RE: [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM

[Yoshihiro Shimoda]

Hi Ulf,

> From: Ulf Hansson, Sent: Monday, June 8, 2020 4:57 PM
> 
> On Tue, 19 May 2020 at 13:33, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> > The commit 432356793415 ("mmc: core: Enable power_off_notify for
> > eMMC shutdown sequence") enabled the power off notification
> > even if MMC_CAP2_POWEROFF_NOTIFY (MMC_CAP2_FULL_PWR_CYCLE now) is
> > not set. However, the mmc core lacks to issue the power off
> > notificaiton when Suspend-to-{RAM,Disk} happens on the system.
> 
> This isn't an entirely correct description, I think.
> 
> If the host supports MMC_CAP2_FULL_PWR_CYCLE (both vmmc and vqmmc can
> be powered on/off), we use power-off-notification during system
> suspend, in case the eMMC card also supports it. Otherwise we send the
> sleep command.

Yes.

> This behaviour was decided on purpose and it's mainly because without
> MMC_CAP2_FULL_PWR_CYCLE, we assume that vqmmc remains always-on. In
> this case, it simply seemed better to use the sleep command, rather
> than the power-off-notification, as we aren't really going to do a
> full power off anyway.

I understood it. However, on my environment (r8a77951-salvator-xs),
while the board is entering Suspend-to-RAM, the vqmmc and vcc are turned off.
Should I add a new flag for such environment?

Best regards,
Yoshihiro Shimoda