[PATCH 1/2] mmc: sdhci-pltfm: fix abort due to missing runtime PM

[PATCH 1/2] mmc: sdhci-pltfm: fix abort due to missing runtime PM

[Stefan Agner]

When entering suspend while the device is in runtime PM, the
sdhci_[suspend|resume]_host function are called without taking
care of runtime PM. On Vybrid SoC this leads to external abort
since during runtime suspend, the clocks for this peripheral
are disabled.

[   37.772967] Unhandled fault: imprecise external abort (0x1c06) at 0x76f5f000
[   37.780304] Internal error: : 1c06 [#1] ARM
[   37.784670] Modules linked in:
[   37.787908] CPU: 0 PID: 428 Comm: sh Not tainted 3.18.0-rc5-00119-geefd097-dirty #1540
[   37.796142] task: 8e246c00 ti: 8ca6c000 task.ti: 8ca6c000
[   37.801785] PC is at esdhc_writel_le+0x40/0xec
[   37.806431] LR is at sdhci_set_card_detection+0xe0/0xe4
[   37.811877] pc : [<803f0584>]    lr : [<803eaaa0>]    psr: 400f0013
[   37.811877] sp : 8ca6dd28  ip : 00000001  fp : 8ca6dd3c
[   37.823766] r10: 807a233c  r9 : 00000000  r8 : 8e8b7210
[   37.829194] r7 : 802d8a08  r6 : 8082e928  r5 : 00000000  r4 : 00000002
[   37.835974] r3 : 8ea34e90  r2 : 00000038  r1 : 00000000  r0 : 8ea32ac0
...

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Not really sure this is the right place to fix this. The function now
ended up to be identically to the implementations in sdhci-pxav3.c...

 drivers/mmc/host/sdhci-pltfm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index c5b01d6..0bc864e 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -31,6 +31,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #ifdef CONFIG_PPC
 #include <asm/machdep.h>
 #endif
@@ -237,17 +238,29 @@ EXPORT_SYMBOL_GPL(sdhci_pltfm_unregister);
 #ifdef CONFIG_PM
 int sdhci_pltfm_suspend(struct device *dev)
 {
+	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
 
-	return sdhci_suspend_host(host);
+	pm_runtime_get_sync(dev);
+	ret = sdhci_suspend_host(host);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_suspend);
 
 int sdhci_pltfm_resume(struct device *dev)
 {
+	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
 
-	return sdhci_resume_host(host);
+	pm_runtime_get_sync(dev);
+	ret = sdhci_resume_host(host);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_resume);
 
-- 
2.1.3

[PATCH 1/2] mmc: sdhci-pltfm: fix abort due to missing runtime PM

[Stefan Agner]

When entering suspend while the device is in runtime PM, the
sdhci_[suspend|resume]_host function are called without taking
care of runtime PM. On Vybrid SoC this leads to external abort
since during runtime suspend, the clocks for this peripheral
are disabled.

[   37.772967] Unhandled fault: imprecise external abort (0x1c06) at 0x76f5f000
[   37.780304] Internal error: : 1c06 [#1] ARM
[   37.784670] Modules linked in:
[   37.787908] CPU: 0 PID: 428 Comm: sh Not tainted 3.18.0-rc5-00119-geefd097-dirty #1540
[   37.796142] task: 8e246c00 ti: 8ca6c000 task.ti: 8ca6c000
[   37.801785] PC is at esdhc_writel_le+0x40/0xec
[   37.806431] LR is at sdhci_set_card_detection+0xe0/0xe4
[   37.811877] pc : [<803f0584>]    lr : [<803eaaa0>]    psr: 400f0013
[   37.811877] sp : 8ca6dd28  ip : 00000001  fp : 8ca6dd3c
[   37.823766] r10: 807a233c  r9 : 00000000  r8 : 8e8b7210
[   37.829194] r7 : 802d8a08  r6 : 8082e928  r5 : 00000000  r4 : 00000002
[   37.835974] r3 : 8ea34e90  r2 : 00000038  r1 : 00000000  r0 : 8ea32ac0
...

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Not really sure this is the right place to fix this. The function now
ended up to be identically to the implementations in sdhci-pxav3.c...

 drivers/mmc/host/sdhci-pltfm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index c5b01d6..0bc864e 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -31,6 +31,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #ifdef CONFIG_PPC
 #include <asm/machdep.h>
 #endif
@@ -237,17 +238,29 @@ EXPORT_SYMBOL_GPL(sdhci_pltfm_unregister);
 #ifdef CONFIG_PM
 int sdhci_pltfm_suspend(struct device *dev)
 {
+	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
 
-	return sdhci_suspend_host(host);
+	pm_runtime_get_sync(dev);
+	ret = sdhci_suspend_host(host);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_suspend);
 
 int sdhci_pltfm_resume(struct device *dev)
 {
+	int ret;
 	struct sdhci_host *host = dev_get_drvdata(dev);
 
-	return sdhci_resume_host(host);
+	pm_runtime_get_sync(dev);
+	ret = sdhci_resume_host(host);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(sdhci_pltfm_resume);
 
-- 
2.1.3

[PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

[Stefan Agner]

Enable IPG clock for sdio interrupts while runtime PM, since this
clock is needed for register access. The need of this clock has been
verified on Vybrid, but this is probably true for i.MX53 and maybe
others. The need for bus access during runtime suspend has been
introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
while sdhci runtime suspended").

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 587ee0e..b7e9ad1 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
 
 	ret = sdhci_runtime_suspend_host(host);
 
-	if (!sdhci_sdio_irq_enabled(host)) {
+	if (!sdhci_sdio_irq_enabled(host))
 		clk_disable_unprepare(imx_data->clk_per);
-		clk_disable_unprepare(imx_data->clk_ipg);
-	}
+
+	clk_disable_unprepare(imx_data->clk_ipg);
 	clk_disable_unprepare(imx_data->clk_ahb);
 
 	return ret;
@@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
 
-	if (!sdhci_sdio_irq_enabled(host)) {
+	if (!sdhci_sdio_irq_enabled(host))
 		clk_prepare_enable(imx_data->clk_per);
-		clk_prepare_enable(imx_data->clk_ipg);
-	}
+
+	clk_prepare_enable(imx_data->clk_ipg);
 	clk_prepare_enable(imx_data->clk_ahb);
 
 	return sdhci_runtime_resume_host(host);
-- 
2.1.3

[PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

[Stefan Agner]

Enable IPG clock for sdio interrupts while runtime PM, since this
clock is needed for register access. The need of this clock has been
verified on Vybrid, but this is probably true for i.MX53 and maybe
others. The need for bus access during runtime suspend has been
introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
while sdhci runtime suspended").

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 587ee0e..b7e9ad1 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
 
 	ret = sdhci_runtime_suspend_host(host);
 
-	if (!sdhci_sdio_irq_enabled(host)) {
+	if (!sdhci_sdio_irq_enabled(host))
 		clk_disable_unprepare(imx_data->clk_per);
-		clk_disable_unprepare(imx_data->clk_ipg);
-	}
+
+	clk_disable_unprepare(imx_data->clk_ipg);
 	clk_disable_unprepare(imx_data->clk_ahb);
 
 	return ret;
@@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = pltfm_host->priv;
 
-	if (!sdhci_sdio_irq_enabled(host)) {
+	if (!sdhci_sdio_irq_enabled(host))
 		clk_prepare_enable(imx_data->clk_per);
-		clk_prepare_enable(imx_data->clk_ipg);
-	}
+
+	clk_prepare_enable(imx_data->clk_ipg);
 	clk_prepare_enable(imx_data->clk_ahb);
 
 	return sdhci_runtime_resume_host(host);
-- 
2.1.3

Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

[Lucas Stach]

Am Dienstag, den 02.12.2014, 17:47 +0100 schrieb Stefan Agner:
> Enable IPG clock for sdio interrupts while runtime PM, since this
> clock is needed for register access. The need of this clock has been
> verified on Vybrid, but this is probably true for i.MX53 and maybe
> others. The need for bus access during runtime suspend has been
> introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
> while sdhci runtime suspended").
> 
Am I reading this wrong, or is this commit actually doing something
different than what the commit message says?

This does _not_ enable the IPG clock during runtime PM, in fact it is
disabling it while suspending even with SDIO ints enabled, which is
wrong, as no SDIO interrupts will be delivered with this clock disabled.

One thing that seems to be wrong with the current code is that the state
of sdhci_sdio_irq_enabled could change between a runtime suspend and
resume, so to keep the clocks balanced the driver has to remember if it
disabled those two clocks in the suspend path and always enable them in
that case in the resume path.

Regards,
Lucas

> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 587ee0e..b7e9ad1 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
>  
>  	ret = sdhci_runtime_suspend_host(host);
>  
> -	if (!sdhci_sdio_irq_enabled(host)) {
> +	if (!sdhci_sdio_irq_enabled(host))
>  		clk_disable_unprepare(imx_data->clk_per);
> -		clk_disable_unprepare(imx_data->clk_ipg);
> -	}
> +
> +	clk_disable_unprepare(imx_data->clk_ipg);
>  	clk_disable_unprepare(imx_data->clk_ahb);
>  
>  	return ret;
> @@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>  
> -	if (!sdhci_sdio_irq_enabled(host)) {
> +	if (!sdhci_sdio_irq_enabled(host))
>  		clk_prepare_enable(imx_data->clk_per);
> -		clk_prepare_enable(imx_data->clk_ipg);
> -	}
> +
> +	clk_prepare_enable(imx_data->clk_ipg);
>  	clk_prepare_enable(imx_data->clk_ahb);
>  
>  	return sdhci_runtime_resume_host(host);

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

[Lucas Stach]

Am Dienstag, den 02.12.2014, 17:47 +0100 schrieb Stefan Agner:
> Enable IPG clock for sdio interrupts while runtime PM, since this
> clock is needed for register access. The need of this clock has been
> verified on Vybrid, but this is probably true for i.MX53 and maybe
> others. The need for bus access during runtime suspend has been
> introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
> while sdhci runtime suspended").
> 
Am I reading this wrong, or is this commit actually doing something
different than what the commit message says?

This does _not_ enable the IPG clock during runtime PM, in fact it is
disabling it while suspending even with SDIO ints enabled, which is
wrong, as no SDIO interrupts will be delivered with this clock disabled.

One thing that seems to be wrong with the current code is that the state
of sdhci_sdio_irq_enabled could change between a runtime suspend and
resume, so to keep the clocks balanced the driver has to remember if it
disabled those two clocks in the suspend path and always enable them in
that case in the resume path.

Regards,
Lucas

> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 587ee0e..b7e9ad1 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
>  
>  	ret = sdhci_runtime_suspend_host(host);
>  
> -	if (!sdhci_sdio_irq_enabled(host)) {
> +	if (!sdhci_sdio_irq_enabled(host))
>  		clk_disable_unprepare(imx_data->clk_per);
> -		clk_disable_unprepare(imx_data->clk_ipg);
> -	}
> +
> +	clk_disable_unprepare(imx_data->clk_ipg);
>  	clk_disable_unprepare(imx_data->clk_ahb);
>  
>  	return ret;
> @@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>  
> -	if (!sdhci_sdio_irq_enabled(host)) {
> +	if (!sdhci_sdio_irq_enabled(host))
>  		clk_prepare_enable(imx_data->clk_per);
> -		clk_prepare_enable(imx_data->clk_ipg);
> -	}
> +
> +	clk_prepare_enable(imx_data->clk_ipg);
>  	clk_prepare_enable(imx_data->clk_ahb);
>  
>  	return sdhci_runtime_resume_host(host);

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

[Stefan Agner]

On 2014-12-03 10:25, Lucas Stach wrote:
> Am Dienstag, den 02.12.2014, 17:47 +0100 schrieb Stefan Agner:
>> Enable IPG clock for sdio interrupts while runtime PM, since this
>> clock is needed for register access. The need of this clock has been
>> verified on Vybrid, but this is probably true for i.MX53 and maybe
>> others. The need for bus access during runtime suspend has been
>> introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
>> while sdhci runtime suspended").
>>
> Am I reading this wrong, or is this commit actually doing something
> different than what the commit message says?
> 
> This does _not_ enable the IPG clock during runtime PM, in fact it is
> disabling it while suspending even with SDIO ints enabled, which is
> wrong, as no SDIO interrupts will be delivered with this clock disabled.

You are completely right. What I wanted was what is written in the
commit message, but I misinterpreted the code!

=> This patch is invalid.

> One thing that seems to be wrong with the current code is that the state
> of sdhci_sdio_irq_enabled could change between a runtime suspend and
> resume, so to keep the clocks balanced the driver has to remember if it
> disabled those two clocks in the suspend path and always enable them in
> that case in the resume path.

As far as I can see, the only place where that flag changes is in
sdhci_enable_sdio_irq. But this function makes sure runtime PM is off
while changing that, so I guess it's ok....

--
Stefan

> 
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 587ee0e..b7e9ad1 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
>>
>>  	ret = sdhci_runtime_suspend_host(host);
>>
>> -	if (!sdhci_sdio_irq_enabled(host)) {
>> +	if (!sdhci_sdio_irq_enabled(host))
>>  		clk_disable_unprepare(imx_data->clk_per);
>> -		clk_disable_unprepare(imx_data->clk_ipg);
>> -	}
>> +
>> +	clk_disable_unprepare(imx_data->clk_ipg);
>>  	clk_disable_unprepare(imx_data->clk_ahb);
>>
>>  	return ret;
>> @@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>
>> -	if (!sdhci_sdio_irq_enabled(host)) {
>> +	if (!sdhci_sdio_irq_enabled(host))
>>  		clk_prepare_enable(imx_data->clk_per);
>> -		clk_prepare_enable(imx_data->clk_ipg);
>> -	}
>> +
>> +	clk_prepare_enable(imx_data->clk_ipg);
>>  	clk_prepare_enable(imx_data->clk_ahb);
>>
>>  	return sdhci_runtime_resume_host(host);

[PATCH 2/2] mmc: sdhci-esdhc-imx: enable IPG clock for sdio interrupts

[Stefan Agner]

On 2014-12-03 10:25, Lucas Stach wrote:
> Am Dienstag, den 02.12.2014, 17:47 +0100 schrieb Stefan Agner:
>> Enable IPG clock for sdio interrupts while runtime PM, since this
>> clock is needed for register access. The need of this clock has been
>> verified on Vybrid, but this is probably true for i.MX53 and maybe
>> others. The need for bus access during runtime suspend has been
>> introduced with be138554a792 ("mmc: sdhci: allow sdio interrupts
>> while sdhci runtime suspended").
>>
> Am I reading this wrong, or is this commit actually doing something
> different than what the commit message says?
> 
> This does _not_ enable the IPG clock during runtime PM, in fact it is
> disabling it while suspending even with SDIO ints enabled, which is
> wrong, as no SDIO interrupts will be delivered with this clock disabled.

You are completely right. What I wanted was what is written in the
commit message, but I misinterpreted the code!

=> This patch is invalid.

> One thing that seems to be wrong with the current code is that the state
> of sdhci_sdio_irq_enabled could change between a runtime suspend and
> resume, so to keep the clocks balanced the driver has to remember if it
> disabled those two clocks in the suspend path and always enable them in
> that case in the resume path.

As far as I can see, the only place where that flag changes is in
sdhci_enable_sdio_irq. But this function makes sure runtime PM is off
while changing that, so I guess it's ok....

--
Stefan

> 
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
>> index 587ee0e..b7e9ad1 100644
>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>> @@ -1193,10 +1193,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
>>
>>  	ret = sdhci_runtime_suspend_host(host);
>>
>> -	if (!sdhci_sdio_irq_enabled(host)) {
>> +	if (!sdhci_sdio_irq_enabled(host))
>>  		clk_disable_unprepare(imx_data->clk_per);
>> -		clk_disable_unprepare(imx_data->clk_ipg);
>> -	}
>> +
>> +	clk_disable_unprepare(imx_data->clk_ipg);
>>  	clk_disable_unprepare(imx_data->clk_ahb);
>>
>>  	return ret;
>> @@ -1208,10 +1208,10 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>  	struct pltfm_imx_data *imx_data = pltfm_host->priv;
>>
>> -	if (!sdhci_sdio_irq_enabled(host)) {
>> +	if (!sdhci_sdio_irq_enabled(host))
>>  		clk_prepare_enable(imx_data->clk_per);
>> -		clk_prepare_enable(imx_data->clk_ipg);
>> -	}
>> +
>> +	clk_prepare_enable(imx_data->clk_ipg);
>>  	clk_prepare_enable(imx_data->clk_ahb);
>>
>>  	return sdhci_runtime_resume_host(host);

Re: [PATCH 1/2] mmc: sdhci-pltfm: fix abort due to missing runtime PM

[Stefan Agner]

On 2014-12-02 17:47, Stefan Agner wrote:
> When entering suspend while the device is in runtime PM, the
> sdhci_[suspend|resume]_host function are called without taking
> care of runtime PM. On Vybrid SoC this leads to external abort
> since during runtime suspend, the clocks for this peripheral
> are disabled.
> 
> [   37.772967] Unhandled fault: imprecise external abort (0x1c06) at 0x76f5f000
> [   37.780304] Internal error: : 1c06 [#1] ARM
> [   37.784670] Modules linked in:
> [   37.787908] CPU: 0 PID: 428 Comm: sh Not tainted
> 3.18.0-rc5-00119-geefd097-dirty #1540
> [   37.796142] task: 8e246c00 ti: 8ca6c000 task.ti: 8ca6c000
> [   37.801785] PC is at esdhc_writel_le+0x40/0xec
> [   37.806431] LR is at sdhci_set_card_detection+0xe0/0xe4
> [   37.811877] pc : [<803f0584>]    lr : [<803eaaa0>]    psr: 400f0013
> [   37.811877] sp : 8ca6dd28  ip : 00000001  fp : 8ca6dd3c
> [   37.823766] r10: 807a233c  r9 : 00000000  r8 : 8e8b7210
> [   37.829194] r7 : 802d8a08  r6 : 8082e928  r5 : 00000000  r4 : 00000002
> [   37.835974] r3 : 8ea34e90  r2 : 00000038  r1 : 00000000  r0 : 8ea32ac0
> ...
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> Not really sure this is the right place to fix this. The function now
> ended up to be identically to the implementations in sdhci-pxav3.c...

Any thoughts on this?

> 
>  drivers/mmc/host/sdhci-pltfm.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
> index c5b01d6..0bc864e 100644
> --- a/drivers/mmc/host/sdhci-pltfm.c
> +++ b/drivers/mmc/host/sdhci-pltfm.c
> @@ -31,6 +31,7 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #ifdef CONFIG_PPC
>  #include <asm/machdep.h>
>  #endif
> @@ -237,17 +238,29 @@ EXPORT_SYMBOL_GPL(sdhci_pltfm_unregister);
>  #ifdef CONFIG_PM
>  int sdhci_pltfm_suspend(struct device *dev)
>  {
> +	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  
> -	return sdhci_suspend_host(host);
> +	pm_runtime_get_sync(dev);
> +	ret = sdhci_suspend_host(host);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_pltfm_suspend);
>  
>  int sdhci_pltfm_resume(struct device *dev)
>  {
> +	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  
> -	return sdhci_resume_host(host);
> +	pm_runtime_get_sync(dev);
> +	ret = sdhci_resume_host(host);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_pltfm_resume);

[PATCH 1/2] mmc: sdhci-pltfm: fix abort due to missing runtime PM

[Stefan Agner]

On 2014-12-02 17:47, Stefan Agner wrote:
> When entering suspend while the device is in runtime PM, the
> sdhci_[suspend|resume]_host function are called without taking
> care of runtime PM. On Vybrid SoC this leads to external abort
> since during runtime suspend, the clocks for this peripheral
> are disabled.
> 
> [   37.772967] Unhandled fault: imprecise external abort (0x1c06) at 0x76f5f000
> [   37.780304] Internal error: : 1c06 [#1] ARM
> [   37.784670] Modules linked in:
> [   37.787908] CPU: 0 PID: 428 Comm: sh Not tainted
> 3.18.0-rc5-00119-geefd097-dirty #1540
> [   37.796142] task: 8e246c00 ti: 8ca6c000 task.ti: 8ca6c000
> [   37.801785] PC is at esdhc_writel_le+0x40/0xec
> [   37.806431] LR is at sdhci_set_card_detection+0xe0/0xe4
> [   37.811877] pc : [<803f0584>]    lr : [<803eaaa0>]    psr: 400f0013
> [   37.811877] sp : 8ca6dd28  ip : 00000001  fp : 8ca6dd3c
> [   37.823766] r10: 807a233c  r9 : 00000000  r8 : 8e8b7210
> [   37.829194] r7 : 802d8a08  r6 : 8082e928  r5 : 00000000  r4 : 00000002
> [   37.835974] r3 : 8ea34e90  r2 : 00000038  r1 : 00000000  r0 : 8ea32ac0
> ...
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> Not really sure this is the right place to fix this. The function now
> ended up to be identically to the implementations in sdhci-pxav3.c...

Any thoughts on this?

> 
>  drivers/mmc/host/sdhci-pltfm.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
> index c5b01d6..0bc864e 100644
> --- a/drivers/mmc/host/sdhci-pltfm.c
> +++ b/drivers/mmc/host/sdhci-pltfm.c
> @@ -31,6 +31,7 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #ifdef CONFIG_PPC
>  #include <asm/machdep.h>
>  #endif
> @@ -237,17 +238,29 @@ EXPORT_SYMBOL_GPL(sdhci_pltfm_unregister);
>  #ifdef CONFIG_PM
>  int sdhci_pltfm_suspend(struct device *dev)
>  {
> +	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  
> -	return sdhci_suspend_host(host);
> +	pm_runtime_get_sync(dev);
> +	ret = sdhci_suspend_host(host);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_pltfm_suspend);
>  
>  int sdhci_pltfm_resume(struct device *dev)
>  {
> +	int ret;
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  
> -	return sdhci_resume_host(host);
> +	pm_runtime_get_sync(dev);
> +	ret = sdhci_resume_host(host);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sdhci_pltfm_resume);