mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver
Subsystems (3)
PATCH v4 1/1 mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
---
Changes since V3:
Invoking sdhci & cqhci resume if sdhci_host_suspend fails.
Removed condition check before invoking cqhci_resume since its a dummy function.
Changes since V2:
Removed disabling/enabling pwr-irq from system pm ops.
Changes since V1:
Invoking pm_runtime_force_suspend/resume instead of sdhci_msm_runtime_suepend/resume.
--- drivers/mmc/host/sdhci-msm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci-msm.c +45 -2
@@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)return 0 ; } +static int sdhci_msm_suspend (struct device *dev) +{ +struct sdhci_host *host =dev_get_drvdata (dev); +int ret; + +if (host->mmc->caps2 &MMC_CAP2_CQE ) { + ret =cqhci_suspend (host->mmc); +if (ret) +return ret; + } + + ret =sdhci_suspend_host (host); +if (ret) + goto resume_cqhci; + + ret =pm_runtime_force_suspend (dev); +if (!ret) +return ret; + +sdhci_resume_host (host); + +resume_cqhci: +cqhci_resume (host->mmc); +return ret; +} + +static int sdhci_msm_resume (struct device *dev) +{ +struct sdhci_host *host =dev_get_drvdata (dev); +int ret; + + ret =pm_runtime_force_resume (dev); +if (ret) +return ret; + + ret =sdhci_resume_host (host); +if (ret <0 ) +return ret; + + ret =cqhci_resume (host->mmc); +return ret; +} +static const struct dev_pm_ops sdhci_msm_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, - pm_runtime_force_resume) + SET_SYSTEM_SLEEP_PM_OPS(sdhci_msm_suspend, + sdhci_msm_resume)SET_RUNTIME_PM_OPS (sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume,NULL )
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Sajida,
Quoted 58 lines · Shaik Sajida Bhanu
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
Signed-off-by Shaik Sajida Bhanu <[email protected]>Reviewed-by Stephen Boyd <[email protected]>---
Changes since V3:
Invoking sdhci & cqhci resume ifsdhci_host_suspendfails.
Removed condition check before invokingcqhci_resumesince its a dummy function.Changes since V2:
Removed disabling/enabling pwr-irq from system pm ops.Changes since V1:
Invoking pm_runtime_force_suspend/resume instead of sdhci_msm_runtime_suepend/resume.--- drivers/mmc/host/sdhci-msm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)drivers/mmc/host/sdhci-msm.c +26
-0@@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)return 0 ; } +static int sdhci_msm_suspend (struct device *dev) +{ +struct sdhci_host *host =dev_get_drvdata (dev); +int ret; + +if (host->mmc->caps2 &MMC_CAP2_CQE ) { + ret =cqhci_suspend (host->mmc); +if (ret) +return ret; + } + + ret =sdhci_suspend_host (host); +if (ret) + goto resume_cqhci; + + ret =pm_runtime_force_suspend (dev); +if (!ret) +return ret; + +sdhci_resume_host (host); + +resume_cqhci: +cqhci_resume (host->mmc); +return ret; +} +
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) · line 2197
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+static int sdhci_msm_resume (struct device *dev) +{ +struct sdhci_host *host =dev_get_drvdata (dev); +int ret; + + ret =pm_runtime_force_resume (dev); +if (ret) +return ret; + + ret =sdhci_resume_host (host);
I'm observing an issue with this change.
After this step, i find interrupt enable register is zero (even though it's getting set in sdhci_resume_host()) and
resulting in request timeout for very first command in resume path.
Until its root caused, please hold back this change.
Quoted 15 lines
drivers/mmc/host/sdhci-msm.c +9
-2+if (ret <0 ) +return ret; + + ret =cqhci_resume (host->mmc); +return ret; +} +static const struct dev_pm_ops sdhci_msm_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, - pm_runtime_force_resume) + SET_SYSTEM_SLEEP_PM_OPS(sdhci_msm_suspend, + sdhci_msm_resume)SET_RUNTIME_PM_OPS (sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume,NULL )
Quoted 37 lines · Shaik Sajida Bhanu
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
Signed-off-by Shaik Sajida Bhanu <[email protected]>Reviewed-by Stephen Boyd <[email protected]>---
Changes since V3:
Invoking sdhci & cqhci resume ifsdhci_host_suspendfails.
Removed condition check before invokingcqhci_resumesince its a dummy function.Changes since V2:
Removed disabling/enabling pwr-irq from system pm ops.Changes since V1:
Invoking pm_runtime_force_suspend/resume instead of sdhci_msm_runtime_suepend/resume.--- drivers/mmc/host/sdhci-msm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)drivers/mmc/host/sdhci-msm.c +4
-0@@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)return 0 ; } +static int sdhci_msm_suspend (struct device *dev) +{ +struct sdhci_host *host =dev_get_drvdata (dev); +int ret;
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) · line 2175
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ +if (host->mmc->caps2 &MMC_CAP2_CQE ) { + ret =cqhci_suspend (host->mmc); +if (ret) +return ret; + } + + ret =sdhci_suspend_host (host); +if (ret) + goto resume_cqhci;
sdhci_suspend_host() can't be called on a device that has been runtime suspended, as that would lead to accessing device registers when clocks/PM domains are gated.
Depending on how the corresponding cqhci device is managed from a runtime PM point of view, it could also be problematic to call cqhci_suspend().
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) · line 2177
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ + ret =pm_runtime_force_suspend (dev);
It looks to me that perhaps you could make use of solely pm_runtime_force_suspend(), then just skip calling sdhci_suspend|resume_host() altogether. Do you think that could work?
And vice versa for sdhci_msm_resume(), of course.
Quoted 36 lines
drivers/mmc/host/sdhci-msm.c +29
-3+if (!ret) +return ret; + +sdhci_resume_host (host); + +resume_cqhci: +cqhci_resume (host->mmc); +return ret; +} + +static int sdhci_msm_resume (struct device *dev) +{ +struct sdhci_host *host =dev_get_drvdata (dev); +int ret; + + ret =pm_runtime_force_resume (dev); +if (ret) +return ret; + + ret =sdhci_resume_host (host); +if (ret <0 ) +return ret; + + ret =cqhci_resume (host->mmc); +return ret; +} +static const struct dev_pm_ops sdhci_msm_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, - pm_runtime_force_resume) + SET_SYSTEM_SLEEP_PM_OPS(sdhci_msm_suspend, + sdhci_msm_resume)SET_RUNTIME_PM_OPS (sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume,NULL ) --
Quoted 3 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Kind regards
Uffe
A message that is not in the archive (sent off-list or not yet received): 158334039680.7173.16159724456027777605@swboyd.mtv.corp.google.com
Quoted 36 lines · Stephen Boyd
Quoting Ulf Hansson (2020-03-04 07:34:29)
Quoted 26 lines · Shaik Sajida Bhanu
Quoted 25 lines
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
Signed-off-by Shaik Sajida Bhanu <[email protected]>Reviewed-by Stephen Boyd <[email protected]>---
Changes since V3:
Invoking sdhci & cqhci resume ifsdhci_host_suspendfails.
Removed condition check before invokingcqhci_resumesince its a dummy function.Changes since V2:
Removed disabling/enabling pwr-irq from system pm ops.Changes since V1:
Invoking pm_runtime_force_suspend/resume instead of sdhci_msm_runtime_suepend/resume.drivers/mmc/host/sdhci-msm.c +0
-0@@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)[...]
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) · line 2175
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ + ret =sdhci_suspend_host (host); +if (ret) + goto resume_cqhci;
sdhci_suspend_host()can't be called on a device that has been runtime suspended, as that would lead to accessing device registers when clocks/PM domains are gated.
Quoted 6 lines
Quoted 3 lines
Depending on how the corresponding cqhci device is managed from a runtime PM point of view, it could also be problematic to call
cqhci_suspend().There seems to be another patch floating around here[1] that is an attempt at a fix to this patch. They should probably be combined so that it's not confusing what's going on.
The other fix is altogether different. It is the fix for the issue seen with run-time pm.
whereas this change is for system pm.
Quoted 10 lines
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ + ret =pm_runtime_force_suspend (dev);It looks to me that perhaps you could make use of solely
pm_runtime_force_suspend(), then just skip calling sdhci_suspend|resume_host() altogether. Do you think that could work?Does that do all the things the commit text mentions is desired for system suspend?
Quoted 1 line
Quoted 1 line
like disabling controller, disabling card detection, enabling wake-up events.
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/
Quoted 39 lines · Stephen Boyd
Quoting Ulf Hansson (2020-03-04 07:34:29)
Quoted 27 lines · Shaik Sajida Bhanu
Quoted 26 lines
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
Signed-off-by Shaik Sajida Bhanu <[email protected]>Reviewed-by Stephen Boyd <[email protected]>---
Changes since V3:
Invoking sdhci & cqhci resume ifsdhci_host_suspendfails.
Removed condition check before invokingcqhci_resumesince its a dummy function.Changes since V2:
Removed disabling/enabling pwr-irq from system pm ops.Changes since V1:
Invoking pm_runtime_force_suspend/resume instead of sdhci_msm_runtime_suepend/resume.drivers/mmc/host/sdhci-msm.c +0
-0@@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)[...]
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) · line 2175
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ + ret =sdhci_suspend_host (host); +if (ret) + goto resume_cqhci;
sdhci_suspend_host()can't be called on a device that has been runtime suspended, as that would lead to accessing device registers when clocks/PM domains are gated.
Quoted 7 lines
Quoted 3 lines
Depending on how the corresponding cqhci device is managed from a runtime PM point of view, it could also be problematic to call
cqhci_suspend().There seems to be another patch floating around here[1] that is an attempt at a fix to this patch. They should probably be combined so that it's not confusing what's going on.
Yeah, it would be easier if these are discussed together.
Quoted 11 lines
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ + ret =pm_runtime_force_suspend (dev);It looks to me that perhaps you could make use of solely
pm_runtime_force_suspend(), then just skip calling sdhci_suspend|resume_host() altogether. Do you think that could work?Does that do all the things the commit text mentions is desired for system suspend?
No. :-)
But why is system wakeup needed for an eMMC card?
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
like disabling controller, disabling card detection, enabling wake-up events.
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/
Kind regards
Uffe
A message that is not in the archive (sent off-list or not yet received): 158463974696.152100.8345578995373250448@swboyd.mtv.corp.google.com
Quoted 33 lines · Stephen Boyd
Quoting Ulf Hansson (2020-03-06 02:07:41)
Quoted 15 lines · Stephen Boyd
Quoted 14 lines
Quoting Ulf Hansson (2020-03-04 07:34:29)
Quoted 12 lines · Shaik Sajida Bhanu
Quoted 11 lines
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
Signed-off-by Shaik Sajida Bhanu <[email protected]>Reviewed-by Stephen Boyd <[email protected]>---
[...]
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
(quoted diff) +2
-0+ + ret = pm_runtime_force_suspend(dev);Quoted 6 lines
Quoted 3 lines
It looks to me that perhaps you could make use of solely
pm_runtime_force_suspend(), then just skip calling sdhci_suspend|resume_host() altogether. Do you think that could work?Does that do all the things the commit text mentions is desired for system suspend?
No. :-)
Quoted 8 lines
Quoted 2 lines
But why is system wakeup needed for an eMMC card?
I don't know if system wakeup is needed for an eMMC card. Probably only if you plug in a card and some daemon wants to wake up and probe the card for auto-play or something like that? Seems possible so might as well expose the CD gpio as a wakeup in that case and let userspace decide if it wants to do that.
Right, card detect IRQs could be useful for system wakeups.
I assume you are using a GPIO IRQ for that, which is easily managed, as the runtime PM status of the mmc controller is irrelevant when configuring the GPIO IRQ as wakeup.
We even have a helper for doing this, mmc_gpio_set_cd_wake().
Quoted 8 lines
Is runtime suspended state the same as system suspended state here though? The commit text seems to imply that only clks are disabled when it's desirable to disable the entire controller. I'm still fuzzy on how runtime PM and system PM interact because it seems to have changed since I looked last a few years ago. If the driver can stay in a runtime suspended state across system suspend then I'm all for it. That would save time for system PM transitions.
In most cases this should be possible. And so far, for this case, I haven't found a good reason to why it shouldn't work.
Although, perhaps we need to improve some of the sdhci's library functions for PM, to better support this.
Kind regards
Uffe
A message that is not in the archive (sent off-list or not yet received): 158690616084.105027.4255268086188981149@swboyd.mtv.corp.google.com
Quoted 47 lines · Stephen Boyd
Quoting Ulf Hansson (2020-03-20 03:22:01)
Quoted 45 lines · Stephen Boyd
Quoted 33 lines
Quoting Ulf Hansson (2020-03-06 02:07:41)
Quoted 15 lines · Stephen Boyd
Quoted 14 lines
Quoting Ulf Hansson (2020-03-04 07:34:29)
Quoted 12 lines · Shaik Sajida Bhanu
Quoted 11 lines
The existing suspend/resume callbacks of sdhci-msm driver are just gating/un-gating the clocks. During suspend cycle more can be done like disabling controller, disabling card detection, enabling wake-up events.
So updating the system pm callbacks for performing these extra actions besides controlling the clocks.
Signed-off-by Shaik Sajida Bhanu <[email protected]>Reviewed-by Stephen Boyd <[email protected]>---
[...]
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
(quoted diff) +2
-0+ + ret = pm_runtime_force_suspend(dev);Quoted 6 lines
Quoted 3 lines
It looks to me that perhaps you could make use of solely
pm_runtime_force_suspend(), then just skip calling sdhci_suspend|resume_host() altogether. Do you think that could work?Does that do all the things the commit text mentions is desired for system suspend?
No. :-)
Quoted 8 lines
Quoted 2 lines
But why is system wakeup needed for an eMMC card?
I don't know if system wakeup is needed for an eMMC card. Probably only if you plug in a card and some daemon wants to wake up and probe the card for auto-play or something like that? Seems possible so might as well expose the CD gpio as a wakeup in that case and let userspace decide if it wants to do that.
Right, card detect IRQs could be useful for system wakeups.
Quoted 8 lines
Quoted 5 lines
I assume you are using a GPIO IRQ for that, which is easily managed, as the runtime PM status of the mmc controller is irrelevant when configuring the GPIO IRQ as wakeup.
We even have a helper for doing this,
mmc_gpio_set_cd_wake().Right. Maybe
mmc_gpio_set_cd_wake()needs to be called from somewhere in the sdhci core?
Yes, that seems reasonable.
Quoted 14 lines
Quoted 14 lines
Quoted 10 lines
Is runtime suspended state the same as system suspended state here though? The commit text seems to imply that only clks are disabled when it's desirable to disable the entire controller. I'm still fuzzy on how runtime PM and system PM interact because it seems to have changed since I looked last a few years ago. If the driver can stay in a runtime suspended state across system suspend then I'm all for it. That would save time for system PM transitions.
In most cases this should be possible. And so far, for this case, I haven't found a good reason to why it shouldn't work.
Quoted 6 lines
Quoted 3 lines
Although, perhaps we need to improve some of the sdhci's library functions for PM, to better support this.
So does that mean it's all just working then? Nothing to do here except make wakeup irqs for CD work?
Well, if it "works " or not, I am not really sure.
My point is, I think most of the things that need to be managed at system suspend/resume are the same things that need to be managed during runtime suspend/resume (except wakeups). So, rather than implementing a whole bunch of system suspend/resume specific things, why not make use of the runtime suspend/resume callbacks instead.
Kind regards
Uffe