* [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver @ 2020-02-20 6:45 Shaik Sajida Bhanu 2020-02-27 14:30 ` Veerabhadrarao Badiganti 2020-03-04 15:34 ` Ulf Hansson 0 siblings, 2 replies; 7+ messages in thread From: Shaik Sajida Bhanu @ 2020-02-20 6:45 UTC (permalink / raw) To: adrian.hunter, ulf.hansson, robh+dt, mka Cc: asutoshd, swboyd, stummala, sayalil, cang, vbadigan, rampraka, linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, bjorn.andersson, 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 <sbhanu@codeaurora.org> Reviewed-by: Stephen Boyd <swboyd@chromium.org> --- 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(-) diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 3955fa5d..3559b50 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -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) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver 2020-02-20 6:45 [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver Shaik Sajida Bhanu @ 2020-02-27 14:30 ` Veerabhadrarao Badiganti 2020-03-04 15:34 ` Ulf Hansson 1 sibling, 0 replies; 7+ messages in thread From: Veerabhadrarao Badiganti @ 2020-02-27 14:30 UTC (permalink / raw) To: Shaik Sajida Bhanu, adrian.hunter, ulf.hansson, robh+dt, mka Cc: asutoshd, swboyd, stummala, sayalil, cang, rampraka, linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, bjorn.andersson Hi Sajida, On 2/20/2020 12:15 PM, Shaik Sajida Bhanu wrote: > 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 <sbhanu@codeaurora.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > --- > 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(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 3955fa5d..3559b50 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -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); 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. > + 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver 2020-02-20 6:45 [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver Shaik Sajida Bhanu 2020-02-27 14:30 ` Veerabhadrarao Badiganti @ 2020-03-04 15:34 ` Ulf Hansson [not found] ` <158334039680.7173.16159724456027777605@swboyd.mtv.corp.google.com> 1 sibling, 1 reply; 7+ messages in thread From: Ulf Hansson @ 2020-03-04 15:34 UTC (permalink / raw) To: Shaik Sajida Bhanu Cc: Adrian Hunter, Rob Herring, Matthias Kaehlcke, Asutosh Das, Stephen Boyd, Sahitya Tummala, Sayali Lokhande, cang, Veerabhadrarao Badiganti, Ram Prakash Gupta, linux-mmc, Linux Kernel Mailing List, linux-arm-msm, DTML, Andy Gross, Bjorn Andersson On Thu, 20 Feb 2020 at 07:45, Shaik Sajida Bhanu <sbhanu@codeaurora.org> wrote: > > 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 <sbhanu@codeaurora.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > --- > 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(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 3955fa5d..3559b50 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -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; 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(). > + > + 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. > + 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) > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > Kind regards Uffe ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <158334039680.7173.16159724456027777605@swboyd.mtv.corp.google.com>]
* Re: [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver [not found] ` <158334039680.7173.16159724456027777605@swboyd.mtv.corp.google.com> @ 2020-03-05 13:56 ` Veerabhadrarao Badiganti 2020-03-06 10:07 ` Ulf Hansson 1 sibling, 0 replies; 7+ messages in thread From: Veerabhadrarao Badiganti @ 2020-03-05 13:56 UTC (permalink / raw) To: Stephen Boyd, Shaik Sajida Bhanu, Ulf Hansson Cc: Adrian Hunter, Rob Herring, Matthias Kaehlcke, Asutosh Das, Sahitya Tummala, Sayali Lokhande, cang, Ram Prakash Gupta, linux-mmc, Linux Kernel Mailing List, linux-arm-msm, DTML, Andy Gross, Bjorn Andersson On 3/4/2020 10:16 PM, Stephen Boyd wrote: > Quoting Ulf Hansson (2020-03-04 07:34:29) >> On Thu, 20 Feb 2020 at 07:45, Shaik Sajida Bhanu <sbhanu@codeaurora.org> wrote: >>> 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 <sbhanu@codeaurora.org> >>> Reviewed-by: Stephen Boyd <swboyd@chromium.org> >>> --- >>> 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. >>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>> index 3955fa5d..3559b50 100644 >>> --- a/drivers/mmc/host/sdhci-msm.c >>> +++ b/drivers/mmc/host/sdhci-msm.c >>> @@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) > [...] >>> + >>> + 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(). > 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. >>> + >>> + 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? > >>> like disabling controller, disabling card detection, enabling wake-up events. > [1] https://lore.kernel.org/linux-arm-msm/1583322863-21790-1-git-send-email-vbadigan@codeaurora.org/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver [not found] ` <158334039680.7173.16159724456027777605@swboyd.mtv.corp.google.com> 2020-03-05 13:56 ` Veerabhadrarao Badiganti @ 2020-03-06 10:07 ` Ulf Hansson [not found] ` <158463974696.152100.8345578995373250448@swboyd.mtv.corp.google.com> 1 sibling, 1 reply; 7+ messages in thread From: Ulf Hansson @ 2020-03-06 10:07 UTC (permalink / raw) To: Stephen Boyd, Shaik Sajida Bhanu Cc: Adrian Hunter, Rob Herring, Matthias Kaehlcke, Asutosh Das, Sahitya Tummala, Sayali Lokhande, cang, Veerabhadrarao Badiganti, Ram Prakash Gupta, linux-mmc, Linux Kernel Mailing List, linux-arm-msm, DTML, Andy Gross, Bjorn Andersson On Wed, 4 Mar 2020 at 17:46, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Ulf Hansson (2020-03-04 07:34:29) > > On Thu, 20 Feb 2020 at 07:45, Shaik Sajida Bhanu <sbhanu@codeaurora.org> wrote: > > > > > > 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 <sbhanu@codeaurora.org> > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > > --- > > > 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. > > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > > > index 3955fa5d..3559b50 100644 > > > --- a/drivers/mmc/host/sdhci-msm.c > > > +++ b/drivers/mmc/host/sdhci-msm.c > > > @@ -2159,9 +2159,52 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) > [...] > > > + > > > + 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(). > > 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. > > > > > > + > > > + 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? > > > > like disabling controller, disabling card detection, enabling wake-up events. > > [1] https://lore.kernel.org/linux-arm-msm/1583322863-21790-1-git-send-email-vbadigan@codeaurora.org/ Kind regards Uffe ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <158463974696.152100.8345578995373250448@swboyd.mtv.corp.google.com>]
* Re: [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver [not found] ` <158463974696.152100.8345578995373250448@swboyd.mtv.corp.google.com> @ 2020-03-20 10:22 ` Ulf Hansson [not found] ` <158690616084.105027.4255268086188981149@swboyd.mtv.corp.google.com> 0 siblings, 1 reply; 7+ messages in thread From: Ulf Hansson @ 2020-03-20 10:22 UTC (permalink / raw) To: Stephen Boyd Cc: Shaik Sajida Bhanu, Adrian Hunter, Rob Herring, Matthias Kaehlcke, Asutosh Das, Sahitya Tummala, Sayali Lokhande, cang, Veerabhadrarao Badiganti, Ram Prakash Gupta, linux-mmc, Linux Kernel Mailing List, linux-arm-msm, DTML, Andy Gross, Bjorn Andersson On Thu, 19 Mar 2020 at 18:42, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Ulf Hansson (2020-03-06 02:07:41) > > On Wed, 4 Mar 2020 at 17:46, Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Ulf Hansson (2020-03-04 07:34:29) > > > > On Thu, 20 Feb 2020 at 07:45, Shaik Sajida Bhanu <sbhanu@codeaurora.org> wrote: > > > > > > > > > > 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 <sbhanu@codeaurora.org> > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > > > > --- > [...] > > > > > > > > > > > > > > + > > > > > + 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? > > > > 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(). > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <158690616084.105027.4255268086188981149@swboyd.mtv.corp.google.com>]
* Re: [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver [not found] ` <158690616084.105027.4255268086188981149@swboyd.mtv.corp.google.com> @ 2020-04-20 9:29 ` Ulf Hansson 0 siblings, 0 replies; 7+ messages in thread From: Ulf Hansson @ 2020-04-20 9:29 UTC (permalink / raw) To: Stephen Boyd Cc: Shaik Sajida Bhanu, Adrian Hunter, Rob Herring, Matthias Kaehlcke, Asutosh Das, Sahitya Tummala, Sayali Lokhande, cang, Veerabhadrarao Badiganti, Ram Prakash Gupta, linux-mmc, Linux Kernel Mailing List, linux-arm-msm, DTML, Andy Gross, Bjorn Andersson On Wed, 15 Apr 2020 at 01:16, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Ulf Hansson (2020-03-20 03:22:01) > > On Thu, 19 Mar 2020 at 18:42, Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Ulf Hansson (2020-03-06 02:07:41) > > > > On Wed, 4 Mar 2020 at 17:46, Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > > Quoting Ulf Hansson (2020-03-04 07:34:29) > > > > > > On Thu, 20 Feb 2020 at 07:45, Shaik Sajida Bhanu <sbhanu@codeaurora.org> wrote: > > > > > > > > > > > > > > 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 <sbhanu@codeaurora.org> > > > > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > > > > > > --- > > > [...] > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + 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? > > > > > > > > > > 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(). > > Right. Maybe mmc_gpio_set_cd_wake() needs to be called from somewhere in > the sdhci core? Yes, that seems reasonable. > > > > > > > > > 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. > > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-20 9:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-20 6:45 [PATCH V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver Shaik Sajida Bhanu 2020-02-27 14:30 ` Veerabhadrarao Badiganti 2020-03-04 15:34 ` Ulf Hansson [not found] ` <158334039680.7173.16159724456027777605@swboyd.mtv.corp.google.com> 2020-03-05 13:56 ` Veerabhadrarao Badiganti 2020-03-06 10:07 ` Ulf Hansson [not found] ` <158463974696.152100.8345578995373250448@swboyd.mtv.corp.google.com> 2020-03-20 10:22 ` Ulf Hansson [not found] ` <158690616084.105027.4255268086188981149@swboyd.mtv.corp.google.com> 2020-04-20 9:29 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).