[PATCH 0/3] mmc: sdhci-s3c: fixes and enhancements

[PATCH 0/3] mmc: sdhci-s3c: fixes and enhancements

[Chander Kashyap]

This patchset does as follows:
Patch 1: Enable only required bus clock in case of multiple clock sources.
Patch 2: Add pm_runtime_dont_use_autosuspend() in sdhci_s3c_remove();
Patch 3: Disable clocks in runtime suspend and enable at runtime resume.

Chander Kashyap (3):
  mmc: sdhci-s3c: Enable only required bus clock
  mmc: sdhci-s3c: Fix crash on module insertion for second time
  mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

 drivers/mmc/host/sdhci-s3c.c |   38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] mmc: sdhci-s3c: Enable only required bus clock

[Chander Kashyap]

In case of multiple bus clock sources, all the clock sources were
getting enabled. As only one clock source is needed at the time hence
enable only the required bus clock.

This patch does as follows:
1.	In sdhci_s3c_probe enable only required bus clock source.

2.	Handle the disabling of old bus clock and enables the
	best clock selected in sdhci_s3c_set_clock().

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
 drivers/mmc/host/sdhci-s3c.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 00969ba..0cbb4c2 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
 		 best_src, clock, best);
 
 	/* select the new clock source */
-
 	if (ourhost->cur_clk != best_src) {
 		struct clk *clk = ourhost->clk_bus[best_src];
 
+		clk_enable(clk);
+		clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
+
 		/* turn clock off to card before changing clock source */
 		writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
 
@@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		 */
 		sc->cur_clk = ptr;
 
-		clk_enable(clk);
-
 		dev_info(dev, "clock source %d: %s (%ld Hz)\n",
 			 ptr, name, clk_get_rate(clk));
 	}
@@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		goto err_no_busclks;
 	}
 
+	clk_enable(sc->clk_bus[sc->cur_clk]);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
 	if (!host->ioaddr) {
@@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	return 0;
 
  err_req_regs:
+	clk_disable(sc->clk_bus[sc->cur_clk]);
 	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
 		if (sc->clk_bus[ptr]) {
-			clk_disable(sc->clk_bus[ptr]);
 			clk_put(sc->clk_bus[ptr]);
 		}
 	}
@@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	clk_disable(sc->clk_bus[sc->cur_clk]);
 	for (ptr = 0; ptr < 3; ptr++) {
 		if (sc->clk_bus[ptr]) {
-			clk_disable(sc->clk_bus[ptr]);
 			clk_put(sc->clk_bus[ptr]);
 		}
 	}
-- 
1.7.9.5

[PATCH 2/3] mmc: sdhci-s3c: Fix crash on module insertion for second time

[Chander Kashyap]

If sdhci-s3c driver is built as module, it gives following error if inserted
again after removing. This was happening as pm_runtime_use_autosuspend() is
called in sdhci_s3c_probe() function but in sdhci_s3c_remove() its complementry
pm_runtime_dont_use_autosuspend() is not called.

BUG: spinlock bad magic on CPU#1, insmod/955
 lock: 0xee771368, .magic: 00000000, .owner: insmod/955, .owner_cpu: 1
[<c00147e0>] (unwind_backtrace+0x0/0xf8) from [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4)
[<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4) from [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38)
[<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38) from [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80)
[<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80) from [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c])
[<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c]) from [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40)
[<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40) from [<c0170090>] (__rpm_callback+0x70/0x98)
[<c0170090>] (__rpm_callback+0x70/0x98) from [<c01703f0>] (rpm_suspend+0xf0/0x534)
[<c01703f0>] (rpm_suspend+0xf0/0x534) from [<c0171670>] (__pm_runtime_suspend+0x5c/0x74)
[<c0171670>] (__pm_runtime_suspend+0x5c/0x74) from [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c)
[<c016d018>] (pm_generic_runtime_idle+0x44/0x4c) from [<c0170090>] (__rpm_callback+0x70/0x98)
[<c0170090>] (__rpm_callback+0x70/0x98) from [<c0170984>] (rpm_idle+0xdc/0x18c)
[<c0170984>] (rpm_idle+0xdc/0x18c) from [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c)
[<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c) from [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c])
[<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c]) from [<c016a014>] (platform_drv_probe+0x18/0x1c)

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
 drivers/mmc/host/sdhci-s3c.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 0cbb4c2..3f4518d 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -658,6 +658,7 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, 1);
 
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	clk_disable(sc->clk_bus[sc->cur_clk]);
-- 
1.7.9.5

[PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chander Kashyap]

Perform clock disable/enable in runtime suspend/resume.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
 drivers/mmc/host/sdhci-s3c.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 3f4518d..ffffd51 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -513,7 +513,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		goto err_no_busclks;
 	}
 
+#ifndef CONFIG_PM_RUNTIME
 	clk_enable(sc->clk_bus[sc->cur_clk]);
+#endif
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
@@ -620,10 +622,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	    gpio_is_valid(pdata->ext_cd_gpio))
 		sdhci_s3c_setup_card_detect_gpio(sc);
 
+	clk_disable(sc->clk_io);
 	return 0;
 
  err_req_regs:
+#ifndef CONFIG_PM_RUNTIME
 	clk_disable(sc->clk_bus[sc->cur_clk]);
+#endif
 	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
 		if (sc->clk_bus[ptr]) {
 			clk_put(sc->clk_bus[ptr]);
@@ -656,12 +661,15 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 	if (gpio_is_valid(sc->ext_cd_gpio))
 		gpio_free(sc->ext_cd_gpio);
 
+	clk_enable(sc->clk_io);
 	sdhci_remove_host(host, 1);
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+#ifndef CONFIG_PM_RUNTIME
 	clk_disable(sc->clk_bus[sc->cur_clk]);
+#endif
 	for (ptr = 0; ptr < 3; ptr++) {
 		if (sc->clk_bus[ptr]) {
 			clk_put(sc->clk_bus[ptr]);
@@ -696,15 +704,28 @@ static int sdhci_s3c_resume(struct device *dev)
 static int sdhci_s3c_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	struct clk *busclk = ourhost->clk_io;
+	int ret;
+
+	ret = sdhci_runtime_suspend_host(host);
 
-	return sdhci_runtime_suspend_host(host);
+	clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
+	clk_disable(busclk);
+	return ret;
 }
 
 static int sdhci_s3c_runtime_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	struct clk *busclk = ourhost->clk_io;
+	int ret;
 
-	return sdhci_runtime_resume_host(host);
+	clk_enable(busclk);
+	clk_enable(ourhost->clk_bus[ourhost->cur_clk]);
+	ret = sdhci_runtime_resume_host(host);
+	return ret;
 }
 #endif
 
-- 
1.7.9.5

Re: [PATCH 1/3] mmc: sdhci-s3c: Enable only required bus clock

[Jaehoon Chung]

Hi Chander,

Could you add the error control for clk_enable()?

On 09/14/2012 06:08 PM, Chander Kashyap wrote:
> In case of multiple bus clock sources, all the clock sources were
> getting enabled. As only one clock source is needed at the time hence
> enable only the required bus clock.
> 
> This patch does as follows:
> 1.	In sdhci_s3c_probe enable only required bus clock source.
> 
> 2.	Handle the disabling of old bus clock and enables the
> 	best clock selected in sdhci_s3c_set_clock().
> 
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 00969ba..0cbb4c2 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
>  		 best_src, clock, best);
>  
>  	/* select the new clock source */
> -
>  	if (ourhost->cur_clk != best_src) {
>  		struct clk *clk = ourhost->clk_bus[best_src];
>  
> +		clk_enable(clk);
> +		clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
> +
Did you consider the case that set_clock is assigned the sdhci_cmu_set_clock()?

>  		/* turn clock off to card before changing clock source */
>  		writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
>  
> @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  		 */
>  		sc->cur_clk = ptr;
>  
> -		clk_enable(clk);
> -
>  		dev_info(dev, "clock source %d: %s (%ld Hz)\n",
>  			 ptr, name, clk_get_rate(clk));
>  	}
> @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  		goto err_no_busclks;
>  	}
>  
> +	clk_enable(sc->clk_bus[sc->cur_clk]);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
>  	if (!host->ioaddr) {
> @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	return 0;
>  
>   err_req_regs:
> +	clk_disable(sc->clk_bus[sc->cur_clk]);
>  	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>  		if (sc->clk_bus[ptr]) {
> -			clk_disable(sc->clk_bus[ptr]);
>  			clk_put(sc->clk_bus[ptr]);
>  		}
>  	}
> @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  
>  	pm_runtime_disable(&pdev->dev);
>  
> +	clk_disable(sc->clk_bus[sc->cur_clk]);
>  	for (ptr = 0; ptr < 3; ptr++) {
>  		if (sc->clk_bus[ptr]) {
> -			clk_disable(sc->clk_bus[ptr]);
>  			clk_put(sc->clk_bus[ptr]);
>  		}
>  	}
> 

Re: [PATCH 1/3] mmc: sdhci-s3c: Enable only required bus clock

[Girish K S]

looks good
Reviewed by: Girish KS <girish.shivananjappa@linaro.org>

On 14 September 2012 14:38, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> In case of multiple bus clock sources, all the clock sources were
> getting enabled. As only one clock source is needed at the time hence
> enable only the required bus clock.
>
> This patch does as follows:
> 1.      In sdhci_s3c_probe enable only required bus clock source.
>
> 2.      Handle the disabling of old bus clock and enables the
>         best clock selected in sdhci_s3c_set_clock().
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 00969ba..0cbb4c2 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
>                  best_src, clock, best);
>
>         /* select the new clock source */
> -
>         if (ourhost->cur_clk != best_src) {
>                 struct clk *clk = ourhost->clk_bus[best_src];
>
> +               clk_enable(clk);
> +               clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
> +
>                 /* turn clock off to card before changing clock source */
>                 writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
>
> @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>                  */
>                 sc->cur_clk = ptr;
>
> -               clk_enable(clk);
> -
>                 dev_info(dev, "clock source %d: %s (%ld Hz)\n",
>                          ptr, name, clk_get_rate(clk));
>         }
> @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>                 goto err_no_busclks;
>         }
>
> +       clk_enable(sc->clk_bus[sc->cur_clk]);
> +
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
>         if (!host->ioaddr) {
> @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>         return 0;
>
>   err_req_regs:
> +       clk_disable(sc->clk_bus[sc->cur_clk]);
>         for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>                 if (sc->clk_bus[ptr]) {
> -                       clk_disable(sc->clk_bus[ptr]);
>                         clk_put(sc->clk_bus[ptr]);
>                 }
>         }
> @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>
>         pm_runtime_disable(&pdev->dev);
>
> +       clk_disable(sc->clk_bus[sc->cur_clk]);
>         for (ptr = 0; ptr < 3; ptr++) {
>                 if (sc->clk_bus[ptr]) {
> -                       clk_disable(sc->clk_bus[ptr]);
>                         clk_put(sc->clk_bus[ptr]);
>                 }
>         }
> --
> 1.7.9.5
>

Re: [PATCH 2/3] mmc: sdhci-s3c: Fix crash on module insertion for second time

[Jaehoon Chung]

Looks good to me..

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

On 09/14/2012 06:08 PM, Chander Kashyap wrote:
> If sdhci-s3c driver is built as module, it gives following error if inserted
> again after removing. This was happening as pm_runtime_use_autosuspend() is
> called in sdhci_s3c_probe() function but in sdhci_s3c_remove() its complementry
> pm_runtime_dont_use_autosuspend() is not called.
> 
> BUG: spinlock bad magic on CPU#1, insmod/955
>  lock: 0xee771368, .magic: 00000000, .owner: insmod/955, .owner_cpu: 1
> [<c00147e0>] (unwind_backtrace+0x0/0xf8) from [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4)
> [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4) from [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38)
> [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38) from [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80)
> [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80) from [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c])
> [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c]) from [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40)
> [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40) from [<c0170090>] (__rpm_callback+0x70/0x98)
> [<c0170090>] (__rpm_callback+0x70/0x98) from [<c01703f0>] (rpm_suspend+0xf0/0x534)
> [<c01703f0>] (rpm_suspend+0xf0/0x534) from [<c0171670>] (__pm_runtime_suspend+0x5c/0x74)
> [<c0171670>] (__pm_runtime_suspend+0x5c/0x74) from [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c)
> [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c) from [<c0170090>] (__rpm_callback+0x70/0x98)
> [<c0170090>] (__rpm_callback+0x70/0x98) from [<c0170984>] (rpm_idle+0xdc/0x18c)
> [<c0170984>] (rpm_idle+0xdc/0x18c) from [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c)
> [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c) from [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c])
> [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c]) from [<c016a014>] (platform_drv_probe+0x18/0x1c)
> 
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 0cbb4c2..3f4518d 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -658,6 +658,7 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  
>  	sdhci_remove_host(host, 1);
>  
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
>  	clk_disable(sc->clk_bus[sc->cur_clk]);
> 

Re: [PATCH 2/3] mmc: sdhci-s3c: Fix crash on module insertion for second time

[Girish K S]

good catch
Reviewed by: Girish K S <girish.shivananjappa@linaro.org>

On 14 September 2012 14:38, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> If sdhci-s3c driver is built as module, it gives following error if inserted
> again after removing. This was happening as pm_runtime_use_autosuspend() is
> called in sdhci_s3c_probe() function but in sdhci_s3c_remove() its complementry
> pm_runtime_dont_use_autosuspend() is not called.
>
> BUG: spinlock bad magic on CPU#1, insmod/955
>  lock: 0xee771368, .magic: 00000000, .owner: insmod/955, .owner_cpu: 1
> [<c00147e0>] (unwind_backtrace+0x0/0xf8) from [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4)
> [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4) from [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38)
> [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38) from [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80)
> [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80) from [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c])
> [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c]) from [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40)
> [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40) from [<c0170090>] (__rpm_callback+0x70/0x98)
> [<c0170090>] (__rpm_callback+0x70/0x98) from [<c01703f0>] (rpm_suspend+0xf0/0x534)
> [<c01703f0>] (rpm_suspend+0xf0/0x534) from [<c0171670>] (__pm_runtime_suspend+0x5c/0x74)
> [<c0171670>] (__pm_runtime_suspend+0x5c/0x74) from [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c)
> [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c) from [<c0170090>] (__rpm_callback+0x70/0x98)
> [<c0170090>] (__rpm_callback+0x70/0x98) from [<c0170984>] (rpm_idle+0xdc/0x18c)
> [<c0170984>] (rpm_idle+0xdc/0x18c) from [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c)
> [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c) from [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c])
> [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c]) from [<c016a014>] (platform_drv_probe+0x18/0x1c)
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 0cbb4c2..3f4518d 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -658,6 +658,7 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>
>         sdhci_remove_host(host, 1);
>
> +       pm_runtime_dont_use_autosuspend(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
>         clk_disable(sc->clk_bus[sc->cur_clk]);
> --
> 1.7.9.5
>

Re: [PATCH 1/3] mmc: sdhci-s3c: Enable only required bus clock

[Chander Kashyap]

Hi Jaehoon Chung,

On 14 September 2012 15:25, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Chander,
>
> Could you add the error control for clk_enable()?
It is maintained as original and clk is already checked for error using IS_ERR.
>
> On 09/14/2012 06:08 PM, Chander Kashyap wrote:
>> In case of multiple bus clock sources, all the clock sources were
>> getting enabled. As only one clock source is needed at the time hence
>> enable only the required bus clock.
>>
>> This patch does as follows:
>> 1.    In sdhci_s3c_probe enable only required bus clock source.
>>
>> 2.    Handle the disabling of old bus clock and enables the
>>       best clock selected in sdhci_s3c_set_clock().
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>>  drivers/mmc/host/sdhci-s3c.c |   12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>> index 00969ba..0cbb4c2 100644
>> --- a/drivers/mmc/host/sdhci-s3c.c
>> +++ b/drivers/mmc/host/sdhci-s3c.c
>> @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
>>                best_src, clock, best);
>>
>>       /* select the new clock source */
>> -
>>       if (ourhost->cur_clk != best_src) {
>>               struct clk *clk = ourhost->clk_bus[best_src];
>>
>> +             clk_enable(clk);
>> +             clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
>> +
> Did you consider the case that set_clock is assigned the sdhci_cmu_set_clock()?
Yes considered.
>
>>               /* turn clock off to card before changing clock source */
>>               writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
>>
>> @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>                */
>>               sc->cur_clk = ptr;
>>
>> -             clk_enable(clk);
>> -
>>               dev_info(dev, "clock source %d: %s (%ld Hz)\n",
>>                        ptr, name, clk_get_rate(clk));
>>       }
>> @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>               goto err_no_busclks;
>>       }
>>
>> +     clk_enable(sc->clk_bus[sc->cur_clk]);
>> +
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
>>       if (!host->ioaddr) {
>> @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>       return 0;
>>
>>   err_req_regs:
>> +     clk_disable(sc->clk_bus[sc->cur_clk]);
>>       for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>>               if (sc->clk_bus[ptr]) {
>> -                     clk_disable(sc->clk_bus[ptr]);
>>                       clk_put(sc->clk_bus[ptr]);
>>               }
>>       }
>> @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>>
>>       pm_runtime_disable(&pdev->dev);
>>
>> +     clk_disable(sc->clk_bus[sc->cur_clk]);
>>       for (ptr = 0; ptr < 3; ptr++) {
>>               if (sc->clk_bus[ptr]) {
>> -                     clk_disable(sc->clk_bus[ptr]);
>>                       clk_put(sc->clk_bus[ptr]);
>>               }
>>       }
>>
>



-- 
with warm regards,
Chander Kashyap

Re: [PATCH 1/3] mmc: sdhci-s3c: Enable only required bus clock

[Jaehoon Chung]

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

On 09/14/2012 07:46 PM, Chander Kashyap wrote:
> Hi Jaehoon Chung,
> 
> On 14 September 2012 15:25, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi Chander,
>>
>> Could you add the error control for clk_enable()?
> It is maintained as original and clk is already checked for error using IS_ERR.
>>
>> On 09/14/2012 06:08 PM, Chander Kashyap wrote:
>>> In case of multiple bus clock sources, all the clock sources were
>>> getting enabled. As only one clock source is needed at the time hence
>>> enable only the required bus clock.
>>>
>>> This patch does as follows:
>>> 1.    In sdhci_s3c_probe enable only required bus clock source.
>>>
>>> 2.    Handle the disabling of old bus clock and enables the
>>>       best clock selected in sdhci_s3c_set_clock().
>>>
>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>> ---
>>>  drivers/mmc/host/sdhci-s3c.c |   12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>>> index 00969ba..0cbb4c2 100644
>>> --- a/drivers/mmc/host/sdhci-s3c.c
>>> +++ b/drivers/mmc/host/sdhci-s3c.c
>>> @@ -203,10 +203,12 @@ static void sdhci_s3c_set_clock(struct sdhci_host *host, unsigned int clock)
>>>                best_src, clock, best);
>>>
>>>       /* select the new clock source */
>>> -
>>>       if (ourhost->cur_clk != best_src) {
>>>               struct clk *clk = ourhost->clk_bus[best_src];
>>>
>>> +             clk_enable(clk);
>>> +             clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
>>> +
>> Did you consider the case that set_clock is assigned the sdhci_cmu_set_clock()?
> Yes considered.
>>
>>>               /* turn clock off to card before changing clock source */
>>>               writew(0, host->ioaddr + SDHCI_CLOCK_CONTROL);
>>>
>>> @@ -501,8 +503,6 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>>                */
>>>               sc->cur_clk = ptr;
>>>
>>> -             clk_enable(clk);
>>> -
>>>               dev_info(dev, "clock source %d: %s (%ld Hz)\n",
>>>                        ptr, name, clk_get_rate(clk));
>>>       }
>>> @@ -513,6 +513,8 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>>               goto err_no_busclks;
>>>       }
>>>
>>> +     clk_enable(sc->clk_bus[sc->cur_clk]);
>>> +
>>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>       host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
>>>       if (!host->ioaddr) {
>>> @@ -621,9 +623,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>>       return 0;
>>>
>>>   err_req_regs:
>>> +     clk_disable(sc->clk_bus[sc->cur_clk]);
>>>       for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>>>               if (sc->clk_bus[ptr]) {
>>> -                     clk_disable(sc->clk_bus[ptr]);
>>>                       clk_put(sc->clk_bus[ptr]);
>>>               }
>>>       }
>>> @@ -658,9 +660,9 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>>>
>>>       pm_runtime_disable(&pdev->dev);
>>>
>>> +     clk_disable(sc->clk_bus[sc->cur_clk]);
>>>       for (ptr = 0; ptr < 3; ptr++) {
>>>               if (sc->clk_bus[ptr]) {
>>> -                     clk_disable(sc->clk_bus[ptr]);
>>>                       clk_put(sc->clk_bus[ptr]);
>>>               }
>>>       }
>>>
>>
> 
> 
> 

Re: [PATCH 1/3] mmc: sdhci-s3c: Enable only required bus clock

[Chris Ball]

Hi,

On Fri, Sep 14 2012, Chander Kashyap wrote:
> In case of multiple bus clock sources, all the clock sources were
> getting enabled. As only one clock source is needed at the time hence
> enable only the required bus clock.
>
> This patch does as follows:
> 1.	In sdhci_s3c_probe enable only required bus clock source.
>
> 2.	Handle the disabling of old bus clock and enables the
> 	best clock selected in sdhci_s3c_set_clock().
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>

Thanks, pushed to mmc-next for 3.7.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/3] mmc: sdhci-s3c: Fix crash on module insertion for second time

[Chris Ball]

Hi,

On Fri, Sep 14 2012, Chander Kashyap wrote:
> If sdhci-s3c driver is built as module, it gives following error if inserted
> again after removing. This was happening as pm_runtime_use_autosuspend() is
> called in sdhci_s3c_probe() function but in sdhci_s3c_remove() its complementry
> pm_runtime_dont_use_autosuspend() is not called.
>
> BUG: spinlock bad magic on CPU#1, insmod/955
>  lock: 0xee771368, .magic: 00000000, .owner: insmod/955, .owner_cpu: 1
> [<c00147e0>] (unwind_backtrace+0x0/0xf8) from [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4)
> [<c0136b40>] (do_raw_spin_unlock+0xa4/0xe4) from [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38)
> [<c01be508>] (_raw_spin_unlock_irqrestore+0xc/0x38) from [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80)
> [<c01a9334>] (sdhci_runtime_suspend_host+0x54/0x80) from [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c])
> [<bf0060a8>] (sdhci_s3c_runtime_suspend+0x14/0x38 [sdhci_s3c]) from [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40)
> [<c016cb00>] (pm_generic_runtime_suspend+0x2c/0x40) from [<c0170090>] (__rpm_callback+0x70/0x98)
> [<c0170090>] (__rpm_callback+0x70/0x98) from [<c01703f0>] (rpm_suspend+0xf0/0x534)
> [<c01703f0>] (rpm_suspend+0xf0/0x534) from [<c0171670>] (__pm_runtime_suspend+0x5c/0x74)
> [<c0171670>] (__pm_runtime_suspend+0x5c/0x74) from [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c)
> [<c016d018>] (pm_generic_runtime_idle+0x44/0x4c) from [<c0170090>] (__rpm_callback+0x70/0x98)
> [<c0170090>] (__rpm_callback+0x70/0x98) from [<c0170984>] (rpm_idle+0xdc/0x18c)
> [<c0170984>] (rpm_idle+0xdc/0x18c) from [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c)
> [<c0171608>] (pm_runtime_set_autosuspend_delay+0x30/0x3c) from [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c])
> [<bf0069c4>] (sdhci_s3c_probe+0x35c/0x52c [sdhci_s3c]) from [<c016a014>] (platform_drv_probe+0x18/0x1c)
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 0cbb4c2..3f4518d 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -658,6 +658,7 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  
>  	sdhci_remove_host(host, 1);
>  
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
>  	clk_disable(sc->clk_bus[sc->cur_clk]);

Thanks, pushed to mmc-next for 3.7.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chris Ball]

Hi Jaehoon, Girish,

On Fri, Sep 14 2012, Chander Kashyap wrote:
> Perform clock disable/enable in runtime suspend/resume.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
>  drivers/mmc/host/sdhci-s3c.c |   25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 3f4518d..ffffd51 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -513,7 +513,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  		goto err_no_busclks;
>  	}
>  
> +#ifndef CONFIG_PM_RUNTIME
>  	clk_enable(sc->clk_bus[sc->cur_clk]);
> +#endif
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
> @@ -620,10 +622,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>  	    gpio_is_valid(pdata->ext_cd_gpio))
>  		sdhci_s3c_setup_card_detect_gpio(sc);
>  
> +	clk_disable(sc->clk_io);
>  	return 0;
>  
>   err_req_regs:
> +#ifndef CONFIG_PM_RUNTIME
>  	clk_disable(sc->clk_bus[sc->cur_clk]);
> +#endif
>  	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>  		if (sc->clk_bus[ptr]) {
>  			clk_put(sc->clk_bus[ptr]);
> @@ -656,12 +661,15 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>  	if (gpio_is_valid(sc->ext_cd_gpio))
>  		gpio_free(sc->ext_cd_gpio);
>  
> +	clk_enable(sc->clk_io);
>  	sdhci_remove_host(host, 1);
>  
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +#ifndef CONFIG_PM_RUNTIME
>  	clk_disable(sc->clk_bus[sc->cur_clk]);
> +#endif
>  	for (ptr = 0; ptr < 3; ptr++) {
>  		if (sc->clk_bus[ptr]) {
>  			clk_put(sc->clk_bus[ptr]);
> @@ -696,15 +704,28 @@ static int sdhci_s3c_resume(struct device *dev)
>  static int sdhci_s3c_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_s3c *ourhost = to_s3c(host);
> +	struct clk *busclk = ourhost->clk_io;
> +	int ret;
> +
> +	ret = sdhci_runtime_suspend_host(host);
>  
> -	return sdhci_runtime_suspend_host(host);
> +	clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
> +	clk_disable(busclk);
> +	return ret;
>  }
>  
>  static int sdhci_s3c_runtime_resume(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_s3c *ourhost = to_s3c(host);
> +	struct clk *busclk = ourhost->clk_io;
> +	int ret;
>  
> -	return sdhci_runtime_resume_host(host);
> +	clk_enable(busclk);
> +	clk_enable(ourhost->clk_bus[ourhost->cur_clk]);
> +	ret = sdhci_runtime_resume_host(host);
> +	return ret;
>  }
>  #endif

Could I get an ACK on this patch from one of you, please?

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Jaehoon Chung]

Looks good to me.

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

On 09/19/2012 03:14 PM, Chris Ball wrote:
> Hi Jaehoon, Girish,
> 
> On Fri, Sep 14 2012, Chander Kashyap wrote:
>> Perform clock disable/enable in runtime suspend/resume.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>>  drivers/mmc/host/sdhci-s3c.c |   25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>> index 3f4518d..ffffd51 100644
>> --- a/drivers/mmc/host/sdhci-s3c.c
>> +++ b/drivers/mmc/host/sdhci-s3c.c
>> @@ -513,7 +513,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>  		goto err_no_busclks;
>>  	}
>>  
>> +#ifndef CONFIG_PM_RUNTIME
>>  	clk_enable(sc->clk_bus[sc->cur_clk]);
>> +#endif
>>  
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
>> @@ -620,10 +622,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>  	    gpio_is_valid(pdata->ext_cd_gpio))
>>  		sdhci_s3c_setup_card_detect_gpio(sc);
>>  
>> +	clk_disable(sc->clk_io);
>>  	return 0;
>>  
>>   err_req_regs:
>> +#ifndef CONFIG_PM_RUNTIME
>>  	clk_disable(sc->clk_bus[sc->cur_clk]);
>> +#endif
>>  	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>>  		if (sc->clk_bus[ptr]) {
>>  			clk_put(sc->clk_bus[ptr]);
>> @@ -656,12 +661,15 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>>  	if (gpio_is_valid(sc->ext_cd_gpio))
>>  		gpio_free(sc->ext_cd_gpio);
>>  
>> +	clk_enable(sc->clk_io);
>>  	sdhci_remove_host(host, 1);
>>  
>>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>>  	pm_runtime_disable(&pdev->dev);
>>  
>> +#ifndef CONFIG_PM_RUNTIME
>>  	clk_disable(sc->clk_bus[sc->cur_clk]);
>> +#endif
>>  	for (ptr = 0; ptr < 3; ptr++) {
>>  		if (sc->clk_bus[ptr]) {
>>  			clk_put(sc->clk_bus[ptr]);
>> @@ -696,15 +704,28 @@ static int sdhci_s3c_resume(struct device *dev)
>>  static int sdhci_s3c_runtime_suspend(struct device *dev)
>>  {
>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>> +	struct sdhci_s3c *ourhost = to_s3c(host);
>> +	struct clk *busclk = ourhost->clk_io;
>> +	int ret;
>> +
>> +	ret = sdhci_runtime_suspend_host(host);
>>  
>> -	return sdhci_runtime_suspend_host(host);
>> +	clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
>> +	clk_disable(busclk);
>> +	return ret;
>>  }
>>  
>>  static int sdhci_s3c_runtime_resume(struct device *dev)
>>  {
>>  	struct sdhci_host *host = dev_get_drvdata(dev);
>> +	struct sdhci_s3c *ourhost = to_s3c(host);
>> +	struct clk *busclk = ourhost->clk_io;
>> +	int ret;
>>  
>> -	return sdhci_runtime_resume_host(host);
>> +	clk_enable(busclk);
>> +	clk_enable(ourhost->clk_bus[ourhost->cur_clk]);
>> +	ret = sdhci_runtime_resume_host(host);
>> +	return ret;
>>  }
>>  #endif
> 
> Could I get an ACK on this patch from one of you, please?
> 
> - Chris.
> 

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chris Ball]

Hi,

On Wed, Sep 19 2012, Jaehoon Chung wrote:
> Looks good to me.
>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

Thanks, pushed to mmc-next for 3.7.  (The patch didn't merge cleanly
so I applied it by hand; let me know if anything looks wrong.)

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chander Kashyap]

Hi Chris,

On 19 September 2012 13:13, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Looks good to me.
>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> On 09/19/2012 03:14 PM, Chris Ball wrote:
>> Hi Jaehoon, Girish,
>>
>> On Fri, Sep 14 2012, Chander Kashyap wrote:
>>> Perform clock disable/enable in runtime suspend/resume.
>>>
>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>> ---
>>>  drivers/mmc/host/sdhci-s3c.c |   25 +++++++++++++++++++++++--
>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
>>> index 3f4518d..ffffd51 100644
>>> --- a/drivers/mmc/host/sdhci-s3c.c
>>> +++ b/drivers/mmc/host/sdhci-s3c.c
>>> @@ -513,7 +513,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>>              goto err_no_busclks;
>>>      }
>>>
>>> +#ifndef CONFIG_PM_RUNTIME
>>>      clk_enable(sc->clk_bus[sc->cur_clk]);
>>> +#endif
>>>
>>>      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>      host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
>>> @@ -620,10 +622,13 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>>>          gpio_is_valid(pdata->ext_cd_gpio))
>>>              sdhci_s3c_setup_card_detect_gpio(sc);
>>>
>>> +    clk_disable(sc->clk_io);
This should be in #ifndef CONFIG_PM_RUNTIME check
>>>      return 0;
>>>
>>>   err_req_regs:
>>> +#ifndef CONFIG_PM_RUNTIME
>>>      clk_disable(sc->clk_bus[sc->cur_clk]);
>>> +#endif
>>>      for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
>>>              if (sc->clk_bus[ptr]) {
>>>                      clk_put(sc->clk_bus[ptr]);
>>> @@ -656,12 +661,15 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
>>>      if (gpio_is_valid(sc->ext_cd_gpio))
>>>              gpio_free(sc->ext_cd_gpio);
>>>
>>> +    clk_enable(sc->clk_io);
This should be in #ifndef CONFIG_PM_RUNTIME check
>>>      sdhci_remove_host(host, 1);
>>>
>>>      pm_runtime_dont_use_autosuspend(&pdev->dev);
>>>      pm_runtime_disable(&pdev->dev);
>>>
>>> +#ifndef CONFIG_PM_RUNTIME
>>>      clk_disable(sc->clk_bus[sc->cur_clk]);
>>> +#endif
>>>      for (ptr = 0; ptr < 3; ptr++) {
>>>              if (sc->clk_bus[ptr]) {
>>>                      clk_put(sc->clk_bus[ptr]);
>>> @@ -696,15 +704,28 @@ static int sdhci_s3c_resume(struct device *dev)
>>>  static int sdhci_s3c_runtime_suspend(struct device *dev)
>>>  {
>>>      struct sdhci_host *host = dev_get_drvdata(dev);
>>> +    struct sdhci_s3c *ourhost = to_s3c(host);
>>> +    struct clk *busclk = ourhost->clk_io;
>>> +    int ret;
>>> +
>>> +    ret = sdhci_runtime_suspend_host(host);
>>>
>>> -    return sdhci_runtime_suspend_host(host);
>>> +    clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
>>> +    clk_disable(busclk);
>>> +    return ret;
>>>  }
>>>
>>>  static int sdhci_s3c_runtime_resume(struct device *dev)
>>>  {
>>>      struct sdhci_host *host = dev_get_drvdata(dev);
>>> +    struct sdhci_s3c *ourhost = to_s3c(host);
>>> +    struct clk *busclk = ourhost->clk_io;
>>> +    int ret;
>>>
>>> -    return sdhci_runtime_resume_host(host);
>>> +    clk_enable(busclk);
>>> +    clk_enable(ourhost->clk_bus[ourhost->cur_clk]);
>>> +    ret = sdhci_runtime_resume_host(host);
>>> +    return ret;
>>>  }
>>>  #endif
>>
>> Could I get an ACK on this patch from one of you, please?
>>
>> - Chris.
>>
>
I will resend this patch after fixing the issue.


-- 
with warm regards,
Chander Kashyap

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chris Ball]

Hi,

On Wed, Sep 19 2012, Chander Kashyap wrote:
> I will resend this patch after fixing the issue.

Thanks; please base your new patch against current mmc-next.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chander Kashyap]

Thanks Chris ,
Sure i will do.

On 19 September 2012 19:51, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Wed, Sep 19 2012, Chander Kashyap wrote:
>> I will resend this patch after fixing the issue.
>
> Thanks; please base your new patch against current mmc-next.
>
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
with warm regards,
Chander Kashyap

[PATCH v2 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chander Kashyap]

From: Chander Kashyap <chander.kashyap@gmail.com>

Perform clock disable/enable in runtime suspend/resume.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
---
Changelog:
v2:
- Wrapped clk_disable in probe and clk_enable in remove with
  #ifdef CONFIG_PM_RUNTIME conditional check.

 drivers/mmc/host/sdhci-s3c.c |   29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index e019672..3726c18 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -637,7 +637,9 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 		goto err_no_busclks;
 	}
 
+#ifndef CONFIG_PM_RUNTIME
 	clk_enable(sc->clk_bus[sc->cur_clk]);
+#endif
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	host->ioaddr = devm_request_and_ioremap(&pdev->dev, res);
@@ -744,10 +746,15 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	    gpio_is_valid(pdata->ext_cd_gpio))
 		sdhci_s3c_setup_card_detect_gpio(sc);
 
+#ifdef CONFIG_PM_RUNTIME
+	clk_disable(sc->clk_io);
+#endif
 	return 0;
 
  err_req_regs:
+#ifndef CONFIG_PM_RUNTIME
 	clk_disable(sc->clk_bus[sc->cur_clk]);
+#endif
 	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
 		if (sc->clk_bus[ptr]) {
 			clk_put(sc->clk_bus[ptr]);
@@ -786,12 +793,17 @@ static int __devexit sdhci_s3c_remove(struct platform_device *pdev)
 	if (gpio_is_valid(sc->ext_cd_gpio))
 		gpio_free(sc->ext_cd_gpio);
 
+#ifdef CONFIG_PM_RUNTIME
+	clk_enable(sc->clk_io);
+#endif
 	sdhci_remove_host(host, 1);
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+#ifndef CONFIG_PM_RUNTIME
 	clk_disable(sc->clk_bus[sc->cur_clk]);
+#endif
 	for (ptr = 0; ptr < MAX_BUS_CLK; ptr++) {
 		if (sc->clk_bus[ptr]) {
 			clk_put(sc->clk_bus[ptr]);
@@ -831,15 +843,28 @@ static int sdhci_s3c_resume(struct device *dev)
 static int sdhci_s3c_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	struct clk *busclk = ourhost->clk_io;
+	int ret;
+
+	ret = sdhci_runtime_suspend_host(host);
 
-	return sdhci_runtime_suspend_host(host);
+	clk_disable(ourhost->clk_bus[ourhost->cur_clk]);
+	clk_disable(busclk);
+	return ret;
 }
 
 static int sdhci_s3c_runtime_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_s3c *ourhost = to_s3c(host);
+	struct clk *busclk = ourhost->clk_io;
+	int ret;
 
-	return sdhci_runtime_resume_host(host);
+	clk_enable(busclk);
+	clk_enable(ourhost->clk_bus[ourhost->cur_clk]);
+	ret = sdhci_runtime_resume_host(host);
+	return ret;
 }
 #endif
 
-- 
1.7.9.5

Re: [PATCH v2 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Chris Ball]

Hi Chander,

On Fri, Sep 21 2012, Chander Kashyap wrote:
> From: Chander Kashyap <chander.kashyap@gmail.com>
>
> Perform clock disable/enable in runtime suspend/resume.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> ---
> Changelog:
> v2:
> - Wrapped clk_disable in probe and clk_enable in remove with
>   #ifdef CONFIG_PM_RUNTIME conditional check.

Thanks, pushed to mmc-next for 3.7.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Heiko Stübner]

Hi,

Am Freitag, 14. September 2012, 11:08:51 schrieb Chander Kashyap:
> Perform clock disable/enable in runtime suspend/resume.
> 
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>

It seems this patch breaks my S3C2416 based machine with 3.7-rc1. I'm not 
yet sure why, but the only response I get is loop of:

	mmc0: Timeout waiting for hardware interrupt.
	mmc0: Internal clock never stabilised.
	mmc0: Timeout waiting for hardware interrupt.
	mmc0: Internal clock never stabilised.


This only happens on the hsmmc channel using the gpio-based card detect and 
even prevents the card from beeing fully detected. The other hsmmc channel 
using a permanent emmc seems to be working fine.

And of course, when I revert this patch everything works fine again.

I'll investigate further, but it'd be also ok if someone has a fix for this 
before me :-) .


Heiko

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Jaehoon Chung]

Hi Heiko,

Sorry, i didn't check this patch with s3c2416.
(i didn't have the s3c2416 board.)
If you have a problem, i think good that revert this patch for fixing your problem.
Also, i will check and share the result.

Best Regards,
Jaehoon Chung

On 10/17/2012 06:15 PM, Heiko Stübner wrote:
> Hi,
> 
> Am Freitag, 14. September 2012, 11:08:51 schrieb Chander Kashyap:
>> Perform clock disable/enable in runtime suspend/resume.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> 
> It seems this patch breaks my S3C2416 based machine with 3.7-rc1. I'm not 
> yet sure why, but the only response I get is loop of:
> 
> 	mmc0: Timeout waiting for hardware interrupt.
> 	mmc0: Internal clock never stabilised.
> 	mmc0: Timeout waiting for hardware interrupt.
> 	mmc0: Internal clock never stabilised.
> 
> 
> This only happens on the hsmmc channel using the gpio-based card detect and 
> even prevents the card from beeing fully detected. The other hsmmc channel 
> using a permanent emmc seems to be working fine.
> 
> And of course, when I revert this patch everything works fine again.
> 
> I'll investigate further, but it'd be also ok if someone has a fix for this 
> before me :-) .
> 
> 
> Heiko
> --
> 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
> 

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Heiko Stübner]

Hi,

Am Donnerstag, 18. Oktober 2012, 04:04:42 schrieb Jaehoon Chung:
> Sorry, i didn't check this patch with s3c2416.
> (i didn't have the s3c2416 board.)
> If you have a problem, i think good that revert this patch for fixing your
> problem. Also, i will check and share the result.


After looking a bit more through the code, I don't think the problem is 2416-
specific but seems to be caused by the gpio card-detect code.

sdhci_s3c_gpio_card_detect_thread calls sdhci_s3c_notify_change which in turn 
runs host->card_tasklet that seems to want to read stuff from the card. But 
this path seems to be missing a runtime-pm wakeup.

I'm not yet sure what to add, especially, as tasklet_finish (called on some 
occasions from tasklet_card) already has a runtime_pm_put call, which would be 
unpaired in this code path.

As there also could be other Samsung platforms affected that use the ext-gpio 
code, it's probably right to revert it for now. Or you see a easy fix :-) .


Heiko


> Best Regards,
> Jaehoon Chung
> 
> On 10/17/2012 06:15 PM, Heiko Stübner wrote:
> > Hi,
> > 
> > Am Freitag, 14. September 2012, 11:08:51 schrieb Chander Kashyap:
> >> Perform clock disable/enable in runtime suspend/resume.
> >> 
> >> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> > 
> > It seems this patch breaks my S3C2416 based machine with 3.7-rc1. I'm not
> > 
> > yet sure why, but the only response I get is loop of:
> > 	mmc0: Timeout waiting for hardware interrupt.
> > 	mmc0: Internal clock never stabilised.
> > 	mmc0: Timeout waiting for hardware interrupt.
> > 	mmc0: Internal clock never stabilised.
> > 
> > This only happens on the hsmmc channel using the gpio-based card detect
> > and even prevents the card from beeing fully detected. The other hsmmc
> > channel using a permanent emmc seems to be working fine.
> > 
> > And of course, when I revert this patch everything works fine again.
> > 
> > I'll investigate further, but it'd be also ok if someone has a fix for
> > this before me :-) .
> > 
> > 
> > Heiko
> > --
> > 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

RE: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Seungwon Jeon]

On Thursday, October 18, 2012, Heiko Stübner <heiko@sntech.de> wrote:
> Hi,
> 
> Am Donnerstag, 18. Oktober 2012, 04:04:42 schrieb Jaehoon Chung:
> > Sorry, i didn't check this patch with s3c2416.
> > (i didn't have the s3c2416 board.)
> > If you have a problem, i think good that revert this patch for fixing your
> > problem. Also, i will check and share the result.
> 
> 
> After looking a bit more through the code, I don't think the problem is 2416-
> specific but seems to be caused by the gpio card-detect code.
> 
> sdhci_s3c_gpio_card_detect_thread calls sdhci_s3c_notify_change which in turn
> runs host->card_tasklet that seems to want to read stuff from the card. But
> this path seems to be missing a runtime-pm wakeup.
> 
> I'm not yet sure what to add, especially, as tasklet_finish (called on some
> occasions from tasklet_card) already has a runtime_pm_put call, which would be
> unpaired in this code path.
> 
> As there also could be other Samsung platforms affected that use the ext-gpio
> code, it's probably right to revert it for now. Or you see a easy fix :-) .
> 
Hi,

I think clock is disabled during bus transaction.
I'll send a patch. Could you test it?

Thanks.

Seungwon Jeon
> 
> Heiko
> 
> 
> > Best Regards,
> > Jaehoon Chung
> >
> > On 10/17/2012 06:15 PM, Heiko Stübner wrote:
> > > Hi,
> > >
> > > Am Freitag, 14. September 2012, 11:08:51 schrieb Chander Kashyap:
> > >> Perform clock disable/enable in runtime suspend/resume.
> > >>
> > >> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> > >
> > > It seems this patch breaks my S3C2416 based machine with 3.7-rc1. I'm not
> > >
> > > yet sure why, but the only response I get is loop of:
> > > 	mmc0: Timeout waiting for hardware interrupt.
> > > 	mmc0: Internal clock never stabilised.
> > > 	mmc0: Timeout waiting for hardware interrupt.
> > > 	mmc0: Internal clock never stabilised.
> > >
> > > This only happens on the hsmmc channel using the gpio-based card detect
> > > and even prevents the card from beeing fully detected. The other hsmmc
> > > channel using a permanent emmc seems to be working fine.
> > >
> > > And of course, when I revert this patch everything works fine again.
> > >
> > > I'll investigate further, but it'd be also ok if someone has a fix for
> > > this before me :-) .
> > >
> > >
> > > Heiko
> > > --
> > > 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
> 
> --
> 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

Re: [PATCH 3/3] mmc: sdhci-s3c: Add clk_(enable/disable) in runtime suspend/resume

[Heiko Stübner]

Am Donnerstag, 18. Oktober 2012, 11:36:03 schrieb Seungwon Jeon:
> On Thursday, October 18, 2012, Heiko Stübner <heiko@sntech.de> wrote:
> > Hi,
> > 
> > Am Donnerstag, 18. Oktober 2012, 04:04:42 schrieb Jaehoon Chung:
> > > Sorry, i didn't check this patch with s3c2416.
> > > (i didn't have the s3c2416 board.)
> > > If you have a problem, i think good that revert this patch for fixing
> > > your problem. Also, i will check and share the result.
> > 
> > After looking a bit more through the code, I don't think the problem is
> > 2416- specific but seems to be caused by the gpio card-detect code.
> > 
> > sdhci_s3c_gpio_card_detect_thread calls sdhci_s3c_notify_change which in
> > turn runs host->card_tasklet that seems to want to read stuff from the
> > card. But this path seems to be missing a runtime-pm wakeup.
> > 
> > I'm not yet sure what to add, especially, as tasklet_finish (called on
> > some occasions from tasklet_card) already has a runtime_pm_put call,
> > which would be unpaired in this code path.
> > 
> > As there also could be other Samsung platforms affected that use the
> > ext-gpio code, it's probably right to revert it for now. Or you see a
> > easy fix :-) .
> 
> Hi,
> 
> I think clock is disabled during bus transaction.
> I'll send a patch. Could you test it?

Sure, I'll test it.

Heiko


> > > On 10/17/2012 06:15 PM, Heiko Stübner wrote:
> > > > Hi,
> > > > 
> > > > Am Freitag, 14. September 2012, 11:08:51 schrieb Chander Kashyap:
> > > >> Perform clock disable/enable in runtime suspend/resume.
> > > >> 
> > > >> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> > > > 
> > > > It seems this patch breaks my S3C2416 based machine with 3.7-rc1. I'm
> > > > not
> > > > 
> > > > yet sure why, but the only response I get is loop of:
> > > > 	mmc0: Timeout waiting for hardware interrupt.
> > > > 	mmc0: Internal clock never stabilised.
> > > > 	mmc0: Timeout waiting for hardware interrupt.
> > > > 	mmc0: Internal clock never stabilised.
> > > > 
> > > > This only happens on the hsmmc channel using the gpio-based card
> > > > detect and even prevents the card from beeing fully detected. The
> > > > other hsmmc channel using a permanent emmc seems to be working fine.
> > > > 
> > > > And of course, when I revert this patch everything works fine again.
> > > > 
> > > > I'll investigate further, but it'd be also ok if someone has a fix
> > > > for this before me :-) .
> > > > 
> > > > 
> > > > Heiko
> > > > --
> > > > 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
> > 
> > --
> > 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