| Message ID | 1579617022-13031-1-git-send-email-sbhanu@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V1] mmc: sdhci-msm: Add system suspend/resume callbacks | expand |
Hi Shaik, On Tue, Jan 21, 2020 at 08:00:22PM +0530, Shaik Sajida Bhanu wrote: > Add system suspend/resume callbacks to sdhci-msm platform driver. > > Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org> > --- > 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 71f29ba..4984857 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -2028,9 +2028,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); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret = 0; initialization is not needed. > + > + if (host->mmc->caps2 & MMC_CAP2_CQE) { > + ret = cqhci_suspend(host->mmc); > + if (ret) > + return ret; > + } > + > + disable_irq(msm_host->pwr_irq); > + ret = sdhci_suspend_host(host); > + if (ret) > + return ret; > + > + return sdhci_msm_runtime_suspend(dev); > +} > + > +static int sdhci_msm_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret = 0; initialization is not needed. > + ret = sdhci_msm_runtime_resume(dev); > + if (ret) > + return ret; > + > + ret = sdhci_resume_host(host); > + if (ret < 0) > + return ret; > + enable_irq(msm_host->pwr_irq); > + > + if (host->mmc->caps2 & MMC_CAP2_CQE) > + 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 21/01/20 4:30 pm, Shaik Sajida Bhanu wrote: > Add system suspend/resume callbacks to sdhci-msm platform driver. There were already callbacks, so the commit subject and messages really do not tell what this change is about or why it is needed. Please explain some more. > > Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org> > --- > 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 71f29ba..4984857 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -2028,9 +2028,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); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret = 0; > + > + if (host->mmc->caps2 & MMC_CAP2_CQE) { > + ret = cqhci_suspend(host->mmc); > + if (ret) > + return ret; > + } > + > + disable_irq(msm_host->pwr_irq); > + ret = sdhci_suspend_host(host); > + if (ret) > + return ret; > + > + return sdhci_msm_runtime_suspend(dev); > +} > + > +static int sdhci_msm_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret = 0; > + > + ret = sdhci_msm_runtime_resume(dev); > + if (ret) > + return ret; > + > + ret = sdhci_resume_host(host); > + if (ret < 0) > + return ret; > + enable_irq(msm_host->pwr_irq); > + > + if (host->mmc->caps2 & MMC_CAP2_CQE) > + 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) >
Quoting Shaik Sajida Bhanu (2020-01-21 06:30:22) > Add system suspend/resume callbacks to sdhci-msm platform driver. Yes, but why? There are already suspend/resume callbacks so this is replacing them too. > > Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org> > --- > 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 71f29ba..4984857 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -2028,9 +2028,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); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret = 0; > + > + if (host->mmc->caps2 & MMC_CAP2_CQE) { It would be nice if this if check was rolled into cqhci_suspend so that all the callers wouldn't have to check it. > + ret = cqhci_suspend(host->mmc); > + if (ret) > + return ret; > + } > + > + disable_irq(msm_host->pwr_irq); Why is the irq disabled? Please add a comment. > + ret = sdhci_suspend_host(host); > + if (ret) > + return ret; > + > + return sdhci_msm_runtime_suspend(dev); pm_runtime_force_suspend() does different things than just call the runtime suspend function for the driver. For example, it disables runtime PM on the device. Can you explain in the commit text how this is a correct conversion? > +} > + > +static int sdhci_msm_resume(struct device *dev) > +{ > + struct sdhci_host *host = dev_get_drvdata(dev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); > + int ret = 0; > + > + ret = sdhci_msm_runtime_resume(dev); > + if (ret) > + return ret; > + > + ret = sdhci_resume_host(host); > + if (ret < 0) > + return ret; > + enable_irq(msm_host->pwr_irq); Same question here about irq. Deserves a comment. > + > + if (host->mmc->caps2 & MMC_CAP2_CQE) > + 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)
Hi Shaik,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to ulf.hansson-mmc/next mmc/mmc-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Shaik-Sajida-Bhanu/mmc-sdhci-msm-Add-system-suspend-resume-callbacks/20200124-084227
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 34597c85be987cc731a840fa0c9bb969c92bd986
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/mmc/host/sdhci-msm.c: In function 'sdhci_msm_suspend':
>> drivers/mmc/host/sdhci-msm.c:2043:9: error: implicit declaration of function 'cqhci_suspend'; did you mean 'cpu_do_suspend'? [-Werror=implicit-function-declaration]
ret = cqhci_suspend(host->mmc);
^~~~~~~~~~~~~
cpu_do_suspend
drivers/mmc/host/sdhci-msm.c: In function 'sdhci_msm_resume':
>> drivers/mmc/host/sdhci-msm.c:2073:9: error: implicit declaration of function 'cqhci_resume'; did you mean 'sdhci_reset'? [-Werror=implicit-function-declaration]
ret = cqhci_resume(host->mmc);
^~~~~~~~~~~~
sdhci_reset
cc1: some warnings being treated as errors
vim +2043 drivers/mmc/host/sdhci-msm.c
2034
2035 static int sdhci_msm_suspend(struct device *dev)
2036 {
2037 struct sdhci_host *host = dev_get_drvdata(dev);
2038 struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
2039 struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
2040 int ret = 0;
2041
2042 if (host->mmc->caps2 & MMC_CAP2_CQE) {
> 2043 ret = cqhci_suspend(host->mmc);
2044 if (ret)
2045 return ret;
2046 }
2047
2048 disable_irq(msm_host->pwr_irq);
2049 ret = sdhci_suspend_host(host);
2050 if (ret)
2051 return ret;
2052
2053 return sdhci_msm_runtime_suspend(dev);
2054 }
2055
2056 static int sdhci_msm_resume(struct device *dev)
2057 {
2058 struct sdhci_host *host = dev_get_drvdata(dev);
2059 struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
2060 struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
2061 int ret = 0;
2062
2063 ret = sdhci_msm_runtime_resume(dev);
2064 if (ret)
2065 return ret;
2066
2067 ret = sdhci_resume_host(host);
2068 if (ret < 0)
2069 return ret;
2070 enable_irq(msm_host->pwr_irq);
2071
2072 if (host->mmc->caps2 & MMC_CAP2_CQE)
> 2073 ret = cqhci_resume(host->mmc);
2074
2075 return ret;
2076 }
2077
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
On 2020-01-22 22:27, Stephen Boyd wrote: > Quoting Shaik Sajida Bhanu (2020-01-21 06:30:22) >> Add system suspend/resume callbacks to sdhci-msm platform driver. > > Yes, but why? There are already suspend/resume callbacks so this is > replacing them too. > Sure. Will update the commit text. The present suspend/resume callbacks are only disabling the clocks. Since eMMC/SDcard would be powered off during system suspend, we can do little more like resetting controller, disabling interrupts. So updating the existing callbacks. >> >> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org> >> --- >> 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 71f29ba..4984857 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -2028,9 +2028,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); >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_msm_host *msm_host = >> sdhci_pltfm_priv(pltfm_host); >> + int ret = 0; >> + >> + if (host->mmc->caps2 & MMC_CAP2_CQE) { > > It would be nice if this if check was rolled into cqhci_suspend so that > all the callers wouldn't have to check it. > >> + ret = cqhci_suspend(host->mmc); >> + if (ret) >> + return ret; >> + } >> + >> + disable_irq(msm_host->pwr_irq); > > Why is the irq disabled? Please add a comment. > Will add a comment. During system suspend all SDHC interrupts can be disabled since device would be in power down state. >> + ret = sdhci_suspend_host(host); >> + if (ret) >> + return ret; >> + >> + return sdhci_msm_runtime_suspend(dev); > > pm_runtime_force_suspend() does different things than just call the > runtime suspend function for the driver. For example, it disables > runtime PM on the device. Can you explain in the commit text how this > is > a correct conversion? > Will invoke pm_runtime_force_suspend() here instead of sdhci_msm_runtime_suspend() >> +} >> + >> +static int sdhci_msm_resume(struct device *dev) >> +{ >> + struct sdhci_host *host = dev_get_drvdata(dev); >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_msm_host *msm_host = >> sdhci_pltfm_priv(pltfm_host); >> + int ret = 0; >> + >> + ret = sdhci_msm_runtime_resume(dev); >> + if (ret) >> + return ret; >> + >> + ret = sdhci_resume_host(host); >> + if (ret < 0) >> + return ret; >> + enable_irq(msm_host->pwr_irq); > > Same question here about irq. Deserves a comment. > >> + >> + if (host->mmc->caps2 & MMC_CAP2_CQE) >> + 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)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 71f29ba..4984857 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -2028,9 +2028,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); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + int ret = 0; + + if (host->mmc->caps2 & MMC_CAP2_CQE) { + ret = cqhci_suspend(host->mmc); + if (ret) + return ret; + } + + disable_irq(msm_host->pwr_irq); + ret = sdhci_suspend_host(host); + if (ret) + return ret; + + return sdhci_msm_runtime_suspend(dev); +} + +static int sdhci_msm_resume(struct device *dev) +{ + struct sdhci_host *host = dev_get_drvdata(dev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + int ret = 0; + + ret = sdhci_msm_runtime_resume(dev); + if (ret) + return ret; + + ret = sdhci_resume_host(host); + if (ret < 0) + return ret; + enable_irq(msm_host->pwr_irq); + + if (host->mmc->caps2 & MMC_CAP2_CQE) + 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)
Add system suspend/resume callbacks to sdhci-msm platform driver. Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org> --- drivers/mmc/host/sdhci-msm.c | 47 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)