Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver.
@ 2020-02-07 13:20 Shaik Sajida Bhanu
  2020-02-10 19:41 ` Stephen Boyd
  0 siblings, 1 reply; 3+ messages in thread
From: Shaik Sajida Bhanu @ 2020-02-07 13:20 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson, robh+dt, mka
  Cc: asutoshd, stummala, sayalil, cang, 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, interrupts and card detection.

So updating the system pm callbacks for performing these extra
actions besides controlling the clocks.

Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>

Changes since V1:
	Addressed review comments
---
 drivers/mmc/host/sdhci-msm.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index c3a160c..e30c8a3 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2159,9 +2159,55 @@ 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;
+
+	if (host->mmc->caps2 & MMC_CAP2_CQE) {
+		ret = cqhci_suspend(host->mmc);
+		if (ret)
+			return ret;
+	}
+
+	ret = sdhci_suspend_host(host);
+	if (ret)
+		return ret;
+	/* Disable pwr-irq since SDHC would be inactive */
+	disable_irq(msm_host->pwr_irq);
+
+	return pm_runtime_force_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;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
+	/* Re-enable pwr irq before SDHC gets reset */
+	enable_irq(msm_host->pwr_irq);
+
+	ret = sdhci_resume_host(host);
+	if (ret < 0)
+		return ret;
+
+	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)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver.
  2020-02-07 13:20 [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver Shaik Sajida Bhanu
@ 2020-02-10 19:41 ` Stephen Boyd
  2020-02-11 14:38   ` sbhanu
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2020-02-10 19:41 UTC (permalink / raw)
  To: Shaik Sajida Bhanu, adrian.hunter, mka, robh+dt, ulf.hansson
  Cc: asutoshd, stummala, sayalil, cang, rampraka, linux-mmc,
	linux-kernel, linux-arm-msm, devicetree, agross, bjorn.andersson,
	Shaik Sajida Bhanu

Quoting Shaik Sajida Bhanu (2020-02-07 05:20:50)
> 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, interrupts and card detection.
> 
> So updating the system pm callbacks for performing these extra
> actions besides controlling the clocks.
> 
> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
> 
> Changes since V1:
>         Addressed review comments

Please don't write this. Instead, describe what's actually different so
the reader doesn't have to go figure out what the review comments were.

> ---
>  drivers/mmc/host/sdhci-msm.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index c3a160c..e30c8a3 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2159,9 +2159,55 @@ 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;
> +
> +       if (host->mmc->caps2 & MMC_CAP2_CQE) {
> +               ret = cqhci_suspend(host->mmc);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       ret = sdhci_suspend_host(host);
> +       if (ret)
> +               return ret;
> +       /* Disable pwr-irq since SDHC would be inactive */
> +       disable_irq(msm_host->pwr_irq);

Why do we need to do this? If it's inactive then the irq won't be raised
by the inactive hardware. Given that we're going to suspend the device,
the irq won't matter unless it's marked for wakeup. Please remove this
irq enable/disable logic, or explain why it's really needed.

> +
> +       return pm_runtime_force_suspend(dev);
> +}
> +

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver.
  2020-02-10 19:41 ` Stephen Boyd
@ 2020-02-11 14:38   ` sbhanu
  0 siblings, 0 replies; 3+ messages in thread
From: sbhanu @ 2020-02-11 14:38 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: adrian.hunter, mka, robh+dt, ulf.hansson, asutoshd, stummala,
	sayalil, cang, rampraka, linux-mmc, linux-kernel, linux-arm-msm,
	devicetree, agross, bjorn.andersson, devicetree-owner

On 2020-02-11 01:11, Stephen Boyd wrote:
> Quoting Shaik Sajida Bhanu (2020-02-07 05:20:50)
>> 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, interrupts and card detection.
>> 
>> So updating the system pm callbacks for performing these extra
>> actions besides controlling the clocks.
>> 
>> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
>> 
>> Changes since V1:
>>         Addressed review comments
> 
> Please don't write this. Instead, describe what's actually different so
> the reader doesn't have to go figure out what the review comments were.
> 

sure

>> ---
>>  drivers/mmc/host/sdhci-msm.c | 50 
>> ++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 48 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci-msm.c 
>> b/drivers/mmc/host/sdhci-msm.c
>> index c3a160c..e30c8a3 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -2159,9 +2159,55 @@ 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;
>> +
>> +       if (host->mmc->caps2 & MMC_CAP2_CQE) {
>> +               ret = cqhci_suspend(host->mmc);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       ret = sdhci_suspend_host(host);
>> +       if (ret)
>> +               return ret;
>> +       /* Disable pwr-irq since SDHC would be inactive */
>> +       disable_irq(msm_host->pwr_irq);
> 
> Why do we need to do this? If it's inactive then the irq won't be 
> raised
> by the inactive hardware. Given that we're going to suspend the device,
> the irq won't matter unless it's marked for wakeup. Please remove this
> irq enable/disable logic, or explain why it's really needed.
> 

You are right. This is not needed.
We have checked more on this and interrupt are getting disabled in 
suspend_device_irqs().
Will remove this.

>> +
>> +       return pm_runtime_force_suspend(dev);
>> +}
>> +

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-07 13:20 [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver Shaik Sajida Bhanu
2020-02-10 19:41 ` Stephen Boyd
2020-02-11 14:38   ` sbhanu

Linux-mmc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mmc/0 linux-mmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mmc linux-mmc/ https://lore.kernel.org/linux-mmc \
		linux-mmc@vger.kernel.org
	public-inbox-index linux-mmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git