| Message ID | 1582181100-29914-1-git-send-email-sbhanu@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V4] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver | expand |
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)
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
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/
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
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
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
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)