| Message ID | 1589887988-7362-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [PATCH/RFC] mmc: core: Issue power_off_notify for eMMC Suspend-to-RAM | expand |
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
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 >
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
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
> 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
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
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
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
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
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);
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(-)