[V1] mmc: sdhci-msm: Add system suspend/resume callbacks
diff mbox series

Message ID 1579617022-13031-1-git-send-email-sbhanu@codeaurora.org
State New
Headers show
Series
  • [V1] mmc: sdhci-msm: Add system suspend/resume callbacks
Related show

Commit Message

Shaik Sajida Bhanu Jan. 21, 2020, 2:30 p.m. UTC
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(-)

Comments

Matthias Kaehlcke Jan. 21, 2020, 11:25 p.m. UTC | #1
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)
Hunter, Adrian Jan. 22, 2020, 7:22 a.m. UTC | #2
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)
>
Stephen Boyd Jan. 22, 2020, 4:57 p.m. UTC | #3
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)
kbuild test robot Jan. 25, 2020, 3:28 p.m. UTC | #4
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
Shaik Sajida Bhanu Feb. 6, 2020, 3:44 p.m. UTC | #5
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)

Patch
diff mbox series

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)