* Re: [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks
2020-01-21 14:30 [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks Shaik Sajida Bhanu
@ 2020-01-21 23:25 ` Matthias Kaehlcke
2020-01-22 7:22 ` Adrian Hunter
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Matthias Kaehlcke @ 2020-01-21 23:25 UTC (permalink / raw)
To: Shaik Sajida Bhanu
Cc: ulf.hansson, adrian.hunter, asutoshd, stummala, vbadigan,
sayalil, cang, rampraka, linux-mmc, linux-kernel, linux-arm-msm,
agross, bjorn.andersson
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks
2020-01-21 14:30 [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks Shaik Sajida Bhanu
2020-01-21 23:25 ` Matthias Kaehlcke
@ 2020-01-22 7:22 ` Adrian Hunter
2020-01-22 16:57 ` Stephen Boyd
2020-01-25 15:28 ` kbuild test robot
3 siblings, 0 replies; 6+ messages in thread
From: Adrian Hunter @ 2020-01-22 7:22 UTC (permalink / raw)
To: Shaik Sajida Bhanu, ulf.hansson
Cc: asutoshd, stummala, vbadigan, sayalil, cang, rampraka, linux-mmc,
linux-kernel, linux-arm-msm, agross, bjorn.andersson
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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks
2020-01-21 14:30 [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks Shaik Sajida Bhanu
2020-01-21 23:25 ` Matthias Kaehlcke
2020-01-22 7:22 ` Adrian Hunter
@ 2020-01-22 16:57 ` Stephen Boyd
2020-02-06 15:44 ` sbhanu
2020-01-25 15:28 ` kbuild test robot
3 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2020-01-22 16:57 UTC (permalink / raw)
To: Shaik Sajida Bhanu, adrian.hunter, ulf.hansson
Cc: asutoshd, stummala, vbadigan, sayalil, cang, rampraka, linux-mmc,
linux-kernel, linux-arm-msm, agross, bjorn.andersson,
Shaik Sajida Bhanu
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks
2020-01-22 16:57 ` Stephen Boyd
@ 2020-02-06 15:44 ` sbhanu
0 siblings, 0 replies; 6+ messages in thread
From: sbhanu @ 2020-02-06 15:44 UTC (permalink / raw)
To: Stephen Boyd
Cc: adrian.hunter, ulf.hansson, asutoshd, stummala, vbadigan,
sayalil, cang, rampraka, linux-mmc, linux-kernel, linux-arm-msm,
agross, bjorn.andersson
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks
2020-01-21 14:30 [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks Shaik Sajida Bhanu
` (2 preceding siblings ...)
2020-01-22 16:57 ` Stephen Boyd
@ 2020-01-25 15:28 ` kbuild test robot
3 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2020-01-25 15:28 UTC (permalink / raw)
To: Shaik Sajida Bhanu
Cc: kbuild-all, ulf.hansson, adrian.hunter, asutoshd, stummala,
vbadigan, sayalil, cang, rampraka, linux-mmc, linux-kernel,
linux-arm-msm, agross, bjorn.andersson, Shaik Sajida Bhanu
[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46552 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread