[PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Pramod Gurav]

Provides runtime PM callbacks to enable and disable clock resources
when idle. Also support system PM callbacks to be called during system
suspend and resume.

Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
---
Changes in v3:
- Added CONFIG_PM around runtime pm function.
- Replaced msm suspend/resume with generic function directly
- Use SET_SYSTEM_SLEEP_PM_OPS instead of late version

Changes in v2:
- Moved pm_rutime enabling before adding host
- Handled pm_rutime in remove
- Changed runtime handling with reference from sdhci-of-at91.c

 drivers/mmc/host/sdhci-msm.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 8ef44a2a..0ef4f29 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -18,6 +18,7 @@
 #include <linux/of_device.h>
 #include <linux/delay.h>
 #include <linux/mmc/mmc.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include "sdhci-pltfm.h"
@@ -658,12 +659,26 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		goto clk_disable;
 	}
 
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+
 	ret = sdhci_add_host(host);
 	if (ret)
-		goto clk_disable;
+		goto pm_runtime_disable;
+
+	platform_set_drvdata(pdev, host);
+
+	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
 
+pm_runtime_disable:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
 clk_disable:
 	clk_disable_unprepare(msm_host->clk);
 pclk_disable:
@@ -685,6 +700,11 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 		    0xffffffff);
 
 	sdhci_remove_host(host, dead);
+
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
 	clk_disable_unprepare(msm_host->clk);
 	clk_disable_unprepare(msm_host->pclk);
 	if (!IS_ERR(msm_host->bus_clk))
@@ -693,12 +713,61 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int sdhci_msm_runtime_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;
+
+	ret = sdhci_runtime_suspend_host(host);
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(msm_host->clk);
+	clk_disable_unprepare(msm_host->pclk);
+
+	return 0;
+}
+
+static int sdhci_msm_runtime_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 = clk_prepare_enable(msm_host->clk);
+	if (ret) {
+		dev_err(dev, "clk_enable failed: %d\n", ret);
+		return ret;
+	}
+	ret = clk_prepare_enable(msm_host->pclk);
+	if (ret) {
+		dev_err(dev, "clk_enable failed: %d\n", ret);
+		clk_disable_unprepare(msm_host->clk);
+		return ret;
+	}
+
+	return sdhci_runtime_resume_host(host);
+}
+#endif
+
+static const struct dev_pm_ops sdhci_msm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+					pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume,
+				NULL)
+};
+
 static struct platform_driver sdhci_msm_driver = {
 	.probe = sdhci_msm_probe,
 	.remove = sdhci_msm_remove,
 	.driver = {
 		   .name = "sdhci_msm",
 		   .of_match_table = sdhci_msm_dt_match,
+		   .pm = &sdhci_msm_pm_ops,
 	},
 };
 
-- 
2.9.3

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Adrian Hunter]

On 01/09/16 17:23, Pramod Gurav wrote:
> Provides runtime PM callbacks to enable and disable clock resources
> when idle. Also support system PM callbacks to be called during system
> suspend and resume.
> 
> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>

Can we get some Tested/Reviewed/Acked-by from people using this driver?

> ---
> Changes in v3:
> - Added CONFIG_PM around runtime pm function.
> - Replaced msm suspend/resume with generic function directly
> - Use SET_SYSTEM_SLEEP_PM_OPS instead of late version
> 
> Changes in v2:
> - Moved pm_rutime enabling before adding host
> - Handled pm_rutime in remove
> - Changed runtime handling with reference from sdhci-of-at91.c
> 
>  drivers/mmc/host/sdhci-msm.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 8ef44a2a..0ef4f29 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -18,6 +18,7 @@
>  #include <linux/of_device.h>
>  #include <linux/delay.h>
>  #include <linux/mmc/mmc.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  
>  #include "sdhci-pltfm.h"
> @@ -658,12 +659,26 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		goto clk_disable;
>  	}
>  
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +
>  	ret = sdhci_add_host(host);
>  	if (ret)
> -		goto clk_disable;
> +		goto pm_runtime_disable;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	pm_runtime_put_autosuspend(&pdev->dev);
>  
>  	return 0;
>  
> +pm_runtime_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>  clk_disable:
>  	clk_disable_unprepare(msm_host->clk);
>  pclk_disable:
> @@ -685,6 +700,11 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>  		    0xffffffff);
>  
>  	sdhci_remove_host(host, dead);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +
>  	clk_disable_unprepare(msm_host->clk);
>  	clk_disable_unprepare(msm_host->pclk);
>  	if (!IS_ERR(msm_host->bus_clk))
> @@ -693,12 +713,61 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int sdhci_msm_runtime_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;
> +
> +	ret = sdhci_runtime_suspend_host(host);
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(msm_host->clk);
> +	clk_disable_unprepare(msm_host->pclk);
> +
> +	return 0;
> +}
> +
> +static int sdhci_msm_runtime_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 = clk_prepare_enable(msm_host->clk);
> +	if (ret) {
> +		dev_err(dev, "clk_enable failed: %d\n", ret);
> +		return ret;
> +	}
> +	ret = clk_prepare_enable(msm_host->pclk);
> +	if (ret) {
> +		dev_err(dev, "clk_enable failed: %d\n", ret);
> +		clk_disable_unprepare(msm_host->clk);
> +		return ret;
> +	}
> +
> +	return sdhci_runtime_resume_host(host);
> +}
> +#endif
> +
> +static const struct dev_pm_ops sdhci_msm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +					pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume,
> +				NULL)
> +};
> +
>  static struct platform_driver sdhci_msm_driver = {
>  	.probe = sdhci_msm_probe,
>  	.remove = sdhci_msm_remove,
>  	.driver = {
>  		   .name = "sdhci_msm",
>  		   .of_match_table = sdhci_msm_dt_match,
> +		   .pm = &sdhci_msm_pm_ops,
>  	},
>  };
>  
> 

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Tummala, Sahitya]

Hi Pramod,

On 9/1/2016 7:53 PM, Pramod Gurav wrote:
> Provides runtime PM callbacks to enable and disable clock resources
> when idle. Also support system PM callbacks to be called during system
> suspend and resume.
>
> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
> ---
> Changes in v3:
> - Added CONFIG_PM around runtime pm function.
> - Replaced msm suspend/resume with generic function directly
> - Use SET_SYSTEM_SLEEP_PM_OPS instead of late version
>
> Changes in v2:
> - Moved pm_rutime enabling before adding host
> - Handled pm_rutime in remove
> - Changed runtime handling with reference from sdhci-of-at91.c
>
>   drivers/mmc/host/sdhci-msm.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 8ef44a2a..0ef4f29 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -18,6 +18,7 @@
>   #include <linux/of_device.h>
>   #include <linux/delay.h>
>   #include <linux/mmc/mmc.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/slab.h>
>   
>   #include "sdhci-pltfm.h"
> @@ -658,12 +659,26 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>   		goto clk_disable;
>   	}
>   
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +
>   	ret = sdhci_add_host(host);
>   	if (ret)
> -		goto clk_disable;
> +		goto pm_runtime_disable;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	pm_runtime_put_autosuspend(&pdev->dev);
>   
>   	return 0;
>   
> +pm_runtime_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>   clk_disable:
>   	clk_disable_unprepare(msm_host->clk);
>   pclk_disable:
> @@ -685,6 +700,11 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>   		    0xffffffff);
>   
>   	sdhci_remove_host(host, dead);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +
>   	clk_disable_unprepare(msm_host->clk);
>   	clk_disable_unprepare(msm_host->pclk);
>   	if (!IS_ERR(msm_host->bus_clk))
> @@ -693,12 +713,61 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +#ifdef CONFIG_PM
> +static int sdhci_msm_runtime_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;
> +
> +	ret = sdhci_runtime_suspend_host(host);
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(msm_host->clk);
> +	clk_disable_unprepare(msm_host->pclk);
> +
> +	return 0;
> +}
> +
> +static int sdhci_msm_runtime_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 = clk_prepare_enable(msm_host->clk);
> +	if (ret) {
> +		dev_err(dev, "clk_enable failed: %d\n", ret);
A minor comment - Both error prints related to clock enable are same. 
Better to print the clock name as well to know which clock enable got 
failed.
> +		return ret;
> +	}
> +	ret = clk_prepare_enable(msm_host->pclk);
> +	if (ret) {
> +		dev_err(dev, "clk_enable failed: %d\n", ret);
> +		clk_disable_unprepare(msm_host->clk);
> +		return ret;
> +	}
> +
> +	return sdhci_runtime_resume_host(host);
> +}
> +#endif
> +
> +static const struct dev_pm_ops sdhci_msm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +					pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(sdhci_msm_runtime_suspend, sdhci_msm_runtime_resume,
> +				NULL)
> +};
> +
>   static struct platform_driver sdhci_msm_driver = {
>   	.probe = sdhci_msm_probe,
>   	.remove = sdhci_msm_remove,
>   	.driver = {
>   		   .name = "sdhci_msm",
>   		   .of_match_table = sdhci_msm_dt_match,
> +		   .pm = &sdhci_msm_pm_ops,
>   	},
>   };
>   

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Georgi Djakov]

On 09/08/2016 11:02 AM, Adrian Hunter wrote:
> On 01/09/16 17:23, Pramod Gurav wrote:
>> Provides runtime PM callbacks to enable and disable clock resources
>> when idle. Also support system PM callbacks to be called during system
>> suspend and resume.
>>
>> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
>
> Can we get some Tested/Reviewed/Acked-by from people using this driver?
>

Hi Pramod,
Thanks for the patch. Unfortunately, my db410c board fails to
boot when i apply it.

[    1.778433] mmc0: new HS200 MMC card at address 0001
[    1.783115] mmcblk0: mmc0:0001 DS2008 7.28 GiB
[    1.783337] mmcblk0boot0: mmc0:0001 DS2008 partition 1 4.00 MiB
[    1.787025] mmcblk0boot1: mmc0:0001 DS2008 partition 2 4.00 MiB
[    1.792893] mmcblk0rpmb: mmc0:0001 DS2008 partition 3 4.00 MiB
[    1.802603]  mmcblk0: p1 p2 p3 p4 p5 p6 p7 p8 p9 p10
[    2.693631] blk_update_request: I/O error, dev mmcblk0, sector 462880
[    2.710381] blk_update_request: I/O error, dev mmcblk0, sector 462880
[    2.710443] Buffer I/O error on dev mmcblk0p10, logical block 0, 
async page read
[    2.724827] blk_update_request: I/O error, dev mmcblk0, sector 462881
[    2.724853] Buffer I/O error on dev mmcblk0p10, logical block 1, 
async page read
...

More I/O errors are following and it is unable to mount the rootfs from
the eMMC. When i retried booting, got also the following:

[    2.877149] mmcblk0: error -110 sending status command, retrying
[    2.879408] mmcblk0: error -110 sending status command, retrying
[    2.884436] mmcblk0: error -110 sending status command, aborting
[    2.896826] mmc0: cache flush error -110

BR,
Georgi

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Pramod Gurav]

On 9 September 2016 at 15:30, Tummala, Sahitya <stummala@codeaurora.org> wrote:
> Hi Pramod,
<snip>
>> +       ret = clk_prepare_enable(msm_host->clk);
>> +       if (ret) {
>> +               dev_err(dev, "clk_enable failed: %d\n", ret);
>
> A minor comment - Both error prints related to clock enable are same. Better
> to print the clock name as well to know which clock enable got failed.
>

Thanks Sahitya for comments. Will take care of this in next version.
>> +               return ret;
>> +       }

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Pramod Gurav]

On 9 September 2016 at 15:48, Georgi Djakov <georgi.djakov@linaro.org> wrote:
> On 09/08/2016 11:02 AM, Adrian Hunter wrote:
>>
>> On 01/09/16 17:23, Pramod Gurav wrote:
>>>
>>> Provides runtime PM callbacks to enable and disable clock resources
>>> when idle. Also support system PM callbacks to be called during system
>>> suspend and resume.
>>>
>>> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
>>
>>
>> Can we get some Tested/Reviewed/Acked-by from people using this driver?
>>
>
> Hi Pramod,
> Thanks for the patch. Unfortunately, my db410c board fails to
> boot when i apply it.
>

Thanks Georgi for testing the patch. Its my wrong I did not update my
kernel and continued fixing comments on old kernel.
After spending some time I came to know that below change is causing the issue:

Author: Dong Aisheng <aisheng.dong@nxp.com>
Date:   Tue Jul 12 15:46:17 2016 +0800

    mmc: sdhci: add standard hw auto retuning support

    If HW supports SDHCI_TUNING_MODE_3 which is auto retuning, we won't
    retune during runtime suspend and resume, instead we use Re-tuning
    Request signaled via SDHCI_INT_RETUNE interrupt to do retuning and
    hw auto retuning during data transfer to guarantee the signal sample
    window correction.

    This can avoid a mass of repeatedly retuning during small file system
    data access and improve the performance.

Specially these lines that was added to suspend path:

+       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
+               mmc_retune_needed(host->mmc);

During sdhci setup in msm driver, the host returns the values to set
sdhci auto tuning as supported.
Hence host->tuning_mode is set to SDHCI_TUNING_MODE_3 during setup.
But some how the auto tuning is not happening.
Just to verify my case, I removed the 'if' part in above code and got
the FS mounted.

Is there anything else needed in msm sdhci driver so that the auto
tuning is taken care of?

> [    1.778433] mmc0: new HS200 MMC card at address 0001
> [    1.783115] mmcblk0: mmc0:0001 DS2008 7.28 GiB
> [    1.783337] mmcblk0boot0: mmc0:0001 DS2008 partition 1 4.00 MiB
> [    1.787025] mmcblk0boot1: mmc0:0001 DS2008 partition 2 4.00 MiB
> [    1.792893] mmcblk0rpmb: mmc0:0001 DS2008 partition 3 4.00 MiB
> [    1.802603]  mmcblk0: p1 p2 p3 p4 p5 p6 p7 p8 p9 p10
> [    2.693631] blk_update_request: I/O error, dev mmcblk0, sector 462880
> [    2.710381] blk_update_request: I/O error, dev mmcblk0, sector 462880
> [    2.710443] Buffer I/O error on dev mmcblk0p10, logical block 0, async
> page read
> [    2.724827] blk_update_request: I/O error, dev mmcblk0, sector 462881
> [    2.724853] Buffer I/O error on dev mmcblk0p10, logical block 1, async
> page read
> ...
>
> More I/O errors are following and it is unable to mount the rootfs from
> the eMMC. When i retried booting, got also the following:
>
> [    2.877149] mmcblk0: error -110 sending status command, retrying
> [    2.879408] mmcblk0: error -110 sending status command, retrying
> [    2.884436] mmcblk0: error -110 sending status command, aborting
> [    2.896826] mmc0: cache flush error -110
>
> BR,
> Georgi

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Ulf Hansson]

On 15 September 2016 at 09:59, Pramod Gurav <pramod.gurav@linaro.org> wrote:
> On 9 September 2016 at 15:48, Georgi Djakov <georgi.djakov@linaro.org> wrote:
>> On 09/08/2016 11:02 AM, Adrian Hunter wrote:
>>>
>>> On 01/09/16 17:23, Pramod Gurav wrote:
>>>>
>>>> Provides runtime PM callbacks to enable and disable clock resources
>>>> when idle. Also support system PM callbacks to be called during system
>>>> suspend and resume.
>>>>
>>>> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
>>>
>>>
>>> Can we get some Tested/Reviewed/Acked-by from people using this driver?
>>>
>>
>> Hi Pramod,
>> Thanks for the patch. Unfortunately, my db410c board fails to
>> boot when i apply it.
>>
>
> Thanks Georgi for testing the patch. Its my wrong I did not update my
> kernel and continued fixing comments on old kernel.
> After spending some time I came to know that below change is causing the issue:
>
> Author: Dong Aisheng <aisheng.dong@nxp.com>
> Date:   Tue Jul 12 15:46:17 2016 +0800
>
>     mmc: sdhci: add standard hw auto retuning support
>
>     If HW supports SDHCI_TUNING_MODE_3 which is auto retuning, we won't
>     retune during runtime suspend and resume, instead we use Re-tuning
>     Request signaled via SDHCI_INT_RETUNE interrupt to do retuning and
>     hw auto retuning during data transfer to guarantee the signal sample
>     window correction.
>
>     This can avoid a mass of repeatedly retuning during small file system
>     data access and improve the performance.
>
> Specially these lines that was added to suspend path:
>
> +       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
> +               mmc_retune_needed(host->mmc);
>
> During sdhci setup in msm driver, the host returns the values to set
> sdhci auto tuning as supported.
> Hence host->tuning_mode is set to SDHCI_TUNING_MODE_3 during setup.
> But some how the auto tuning is not happening.
> Just to verify my case, I removed the 'if' part in above code and got
> the FS mounted.
>
> Is there anything else needed in msm sdhci driver so that the auto
> tuning is taken care of?

I am not familiar with any other than sdhci-esdhc-imx which supports
the SDHCI_TUNING_MODE_3. I may be wrong though.

In the sdhci-esdhc-imx case, enabling of auto tuning seems to be done
in esdhc_post_tuning(), where a vendor specific register
(ESDHC_MIX_CTRL) is being written to. Perhaps something similar in
your case?

[...]

Kind regards
Uffe

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Pramod Gurav]

On 15 September 2016 at 15:49, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 15 September 2016 at 09:59, Pramod Gurav <pramod.gurav@linaro.org> wrote:
>> On 9 September 2016 at 15:48, Georgi Djakov <georgi.djakov@linaro.org> wrote:
>>> On 09/08/2016 11:02 AM, Adrian Hunter wrote:
>>>>
>>>> On 01/09/16 17:23, Pramod Gurav wrote:
>>>>>
>>>>> Provides runtime PM callbacks to enable and disable clock resources
>>>>> when idle. Also support system PM callbacks to be called during system
>>>>> suspend and resume.
>>>>>
>>>>> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
>>>>
>>>>
>>>> Can we get some Tested/Reviewed/Acked-by from people using this driver?
>>>>
>>>
>>> Hi Pramod,
>>> Thanks for the patch. Unfortunately, my db410c board fails to
>>> boot when i apply it.
>>>
>>
>> Thanks Georgi for testing the patch. Its my wrong I did not update my
>> kernel and continued fixing comments on old kernel.
>> After spending some time I came to know that below change is causing the issue:
>>
>> Author: Dong Aisheng <aisheng.dong@nxp.com>
>> Date:   Tue Jul 12 15:46:17 2016 +0800
>>
>>     mmc: sdhci: add standard hw auto retuning support
>>
>>     If HW supports SDHCI_TUNING_MODE_3 which is auto retuning, we won't
>>     retune during runtime suspend and resume, instead we use Re-tuning
>>     Request signaled via SDHCI_INT_RETUNE interrupt to do retuning and
>>     hw auto retuning during data transfer to guarantee the signal sample
>>     window correction.
>>
>>     This can avoid a mass of repeatedly retuning during small file system
>>     data access and improve the performance.
>>
>> Specially these lines that was added to suspend path:
>>
>> +       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>> +               mmc_retune_needed(host->mmc);
>>
>> During sdhci setup in msm driver, the host returns the values to set
>> sdhci auto tuning as supported.
>> Hence host->tuning_mode is set to SDHCI_TUNING_MODE_3 during setup.
>> But some how the auto tuning is not happening.
>> Just to verify my case, I removed the 'if' part in above code and got
>> the FS mounted.
>>
>> Is there anything else needed in msm sdhci driver so that the auto
>> tuning is taken care of?
>
> I am not familiar with any other than sdhci-esdhc-imx which supports
> the SDHCI_TUNING_MODE_3. I may be wrong though.
>
> In the sdhci-esdhc-imx case, enabling of auto tuning seems to be done
> in esdhc_post_tuning(), where a vendor specific register
> (ESDHC_MIX_CTRL) is being written to. Perhaps something similar in
> your case?
>
Thanks Ulf for the comments. Will check this and see if there is
something of this sort we have to do to achieve auto tuning.
Adding Ritesh who has been posting some SDHCI MSM patches recently in
case he knows about this.

Regards,
Pramod

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Ritesh Harjani]

Hi Pramod,

On 9/15/2016 7:28 PM, Pramod Gurav wrote:
> On 15 September 2016 at 15:49, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 15 September 2016 at 09:59, Pramod Gurav <pramod.gurav@linaro.org> wrote:
>>> On 9 September 2016 at 15:48, Georgi Djakov <georgi.djakov@linaro.org> wrote:
>>>> On 09/08/2016 11:02 AM, Adrian Hunter wrote:
>>>>>
>>>>> On 01/09/16 17:23, Pramod Gurav wrote:
>>>>>>
>>>>>> Provides runtime PM callbacks to enable and disable clock resources
>>>>>> when idle. Also support system PM callbacks to be called during system
>>>>>> suspend and resume.
>>>>>>
>>>>>> Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
>>>>>
>>>>>
>>>>> Can we get some Tested/Reviewed/Acked-by from people using this driver?
>>>>>
>>>>
>>>> Hi Pramod,
>>>> Thanks for the patch. Unfortunately, my db410c board fails to
>>>> boot when i apply it.
>>>>
>>>
>>> Thanks Georgi for testing the patch. Its my wrong I did not update my
>>> kernel and continued fixing comments on old kernel.
>>> After spending some time I came to know that below change is causing the issue:
>>>
>>> Author: Dong Aisheng <aisheng.dong@nxp.com>
>>> Date:   Tue Jul 12 15:46:17 2016 +0800
>>>
>>>     mmc: sdhci: add standard hw auto retuning support
>>>
>>>     If HW supports SDHCI_TUNING_MODE_3 which is auto retuning, we won't
>>>     retune during runtime suspend and resume, instead we use Re-tuning
>>>     Request signaled via SDHCI_INT_RETUNE interrupt to do retuning and
>>>     hw auto retuning during data transfer to guarantee the signal sample
>>>     window correction.
>>>
>>>     This can avoid a mass of repeatedly retuning during small file system
>>>     data access and improve the performance.
>>>
>>> Specially these lines that was added to suspend path:
>>>
>>> +       if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>>> +               mmc_retune_needed(host->mmc);
>>>
>>> During sdhci setup in msm driver, the host returns the values to set
>>> sdhci auto tuning as supported.
>>> Hence host->tuning_mode is set to SDHCI_TUNING_MODE_3 during setup.
>>> But some how the auto tuning is not happening.
>>> Just to verify my case, I removed the 'if' part in above code and got
>>> the FS mounted.
>>>
>>> Is there anything else needed in msm sdhci driver so that the auto
>>> tuning is taken care of?
>>
>> I am not familiar with any other than sdhci-esdhc-imx which supports
>> the SDHCI_TUNING_MODE_3. I may be wrong though.
>>
>> In the sdhci-esdhc-imx case, enabling of auto tuning seems to be done
>> in esdhc_post_tuning(), where a vendor specific register
>> (ESDHC_MIX_CTRL) is being written to. Perhaps something similar in
>> your case?
>>
> Thanks Ulf for the comments. Will check this and see if there is
> something of this sort we have to do to achieve auto tuning.
> Adding Ritesh who has been posting some SDHCI MSM patches recently in
> case he knows about this.

Internally, we don't use this Auto re-tuning and rely on explicit 
re-tune by host driver.

Question though -
1. why do we need to call sdhci_runtime_resume/suspend from 
sdhci_msm_runtime_suspend/resume?
 From what I see is, sdhci_runtime_susend/resume will do reset and 
re-program of host->pwr and host->clk because of which a retune will be 
required for the next command after runtime resume.

We can *only* disable and enable the clocks in 
sdhci_msm_runtime_suspend/resume?
Thoughts? With this, I suppose you would not see any issue.


Though for this issue, since internally also auto retuning is never 
used, we can have this mode disabled. I can once again check with HW 
team to get more details about this mode for MSM controller.

>
> Regards,
> Pramod
>

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Pramod Gurav]

Hi Ritesh,

Thanks for the inputs.

On 22 September 2016 at 20:02, Ritesh Harjani <riteshh@codeaurora.org> wrote:
> Hi Pramod,

<snip>

>> Thanks Ulf for the comments. Will check this and see if there is
>> something of this sort we have to do to achieve auto tuning.
>> Adding Ritesh who has been posting some SDHCI MSM patches recently in
>> case he knows about this.
>
>
> Internally, we don't use this Auto re-tuning and rely on explicit re-tune by
> host driver.
>
> Question though -
> 1. why do we need to call sdhci_runtime_resume/suspend from
> sdhci_msm_runtime_suspend/resume?
> From what I see is, sdhci_runtime_susend/resume will do reset and re-program
> of host->pwr and host->clk because of which a retune will be required for
> the next command after runtime resume.

Honestly I took reference from existing SDHCI HC driver which
implement the runtime PM and each one uses this function.

>
> We can *only* disable and enable the clocks in
> sdhci_msm_runtime_suspend/resume?
> Thoughts? With this, I suppose you would not see any issue.
This should work as I did not have this funtion call in my V1 but
later included when I referred the other sdhci drivers.

>
>
> Though for this issue, since internally also auto retuning is never used, we
> can have this mode disabled. I can once again check with HW team to get more
> details about this mode for MSM controller.
This seems a read only register. And I could not find any other
reference of this mode in any of the docs.
>
>>
>> Regards,
>> Pramod
>>
>

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Ulf Hansson]

[...]

>>>> Is there anything else needed in msm sdhci driver so that the auto
>>>> tuning is taken care of?
>>>
>>>
>>> I am not familiar with any other than sdhci-esdhc-imx which supports
>>> the SDHCI_TUNING_MODE_3. I may be wrong though.
>>>
>>> In the sdhci-esdhc-imx case, enabling of auto tuning seems to be done
>>> in esdhc_post_tuning(), where a vendor specific register
>>> (ESDHC_MIX_CTRL) is being written to. Perhaps something similar in
>>> your case?
>>>
>> Thanks Ulf for the comments. Will check this and see if there is
>> something of this sort we have to do to achieve auto tuning.
>> Adding Ritesh who has been posting some SDHCI MSM patches recently in
>> case he knows about this.
>
>
> Internally, we don't use this Auto re-tuning and rely on explicit re-tune by
> host driver.
>
> Question though -
> 1. why do we need to call sdhci_runtime_resume/suspend from
> sdhci_msm_runtime_suspend/resume?
> From what I see is, sdhci_runtime_susend/resume will do reset and re-program
> of host->pwr and host->clk because of which a retune will be required for
> the next command after runtime resume.
>
> We can *only* disable and enable the clocks in
> sdhci_msm_runtime_suspend/resume?
> Thoughts? With this, I suppose you would not see any issue.

I see.

I assumes that means saving/restoring register context will
automatically handled by some other outer logic, when doing clock
gating/ungating?

In other words, if the controller has valid tuning values, those will
be re-used and restored when clock ungating happens?

>
>
> Though for this issue, since internally also auto retuning is never used, we
> can have this mode disabled. I can once again check with HW team to get more
> details about this mode for MSM controller.
>
>>
>> Regards,
>> Pramod
>>
>

Kind regards
Uffe

Re: [PATCH v3] mmc: sdhci-msm: Add pm_runtime and system PM support

[Ritesh Harjani]

Hi Ulf,


On 9/23/2016 3:37 PM, Ulf Hansson wrote:
> [...]
>
>>>>> Is there anything else needed in msm sdhci driver so that the auto
>>>>> tuning is taken care of?
>>>>
>>>>
>>>> I am not familiar with any other than sdhci-esdhc-imx which supports
>>>> the SDHCI_TUNING_MODE_3. I may be wrong though.
>>>>
>>>> In the sdhci-esdhc-imx case, enabling of auto tuning seems to be done
>>>> in esdhc_post_tuning(), where a vendor specific register
>>>> (ESDHC_MIX_CTRL) is being written to. Perhaps something similar in
>>>> your case?
>>>>
>>> Thanks Ulf for the comments. Will check this and see if there is
>>> something of this sort we have to do to achieve auto tuning.
>>> Adding Ritesh who has been posting some SDHCI MSM patches recently in
>>> case he knows about this.
>>
>>
>> Internally, we don't use this Auto re-tuning and rely on explicit re-tune by
>> host driver.
>>
>> Question though -
>> 1. why do we need to call sdhci_runtime_resume/suspend from
>> sdhci_msm_runtime_suspend/resume?
>> From what I see is, sdhci_runtime_susend/resume will do reset and re-program
>> of host->pwr and host->clk because of which a retune will be required for
>> the next command after runtime resume.
>>
>> We can *only* disable and enable the clocks in
>> sdhci_msm_runtime_suspend/resume?
>> Thoughts? With this, I suppose you would not see any issue.
>
> I see.
>
> I assumes that means saving/restoring register context will
> automatically handled by some other outer logic, when doing clock
> gating/ungating?
>
> In other words, if the controller has valid tuning values, those will
> be re-used and restored when clock ungating happens?
Yes, that is my understanding too. I double confirmed with HW team about 
this. So, even if we gate the clock directly at GCC, sdhc msm controller 
is capable of restoring it's register values.

In this case, it is not required to call for 
sdhci_runtime_suspend/resume from sdhci_msm_runtime routines right?
Instead we can only have disabling/enabling of clks from 
sdhci_msm_runtime_suspend/resume. Does this sounds good?


>
>>
>>
>> Though for this issue, since internally also auto retuning is never used, we
>> can have this mode disabled. I can once again check with HW team to get more
>> details about this mode for MSM controller.
>>
>>>
>>> Regards,
>>> Pramod
>>>
>>
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>