[PATCH] mmc: tmio: Fix hang during suspend

[PATCH] mmc: tmio: Fix hang during suspend

[Geert Uytterhoeven]

On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
module clock is disabled. Doing so will cause a lock-up.

When suspending, enable the module clock before disabling the SDHI
interrupts, and disable the clock again afterwards.

This fixes suspend and resume on r8a7791/koelsch.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index faf0924e71cb..83192420a7e3 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev)
 {
 	struct mmc_host *mmc = dev_get_drvdata(dev);
 	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct tmio_mmc_data *pdata = host->pdata;
+
+	if (pdata->clk_enable) {
+		unsigned int f;
+		pdata->clk_enable(host->pdev, &f);
+	}
 
 	tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
+
+	if (pdata->clk_disable)
+		pdata->clk_disable(host->pdev);
+
 	return 0;
 }
 EXPORT_SYMBOL(tmio_mmc_host_suspend);
-- 
1.9.1

Re: [PATCH] mmc: tmio: Fix hang during suspend

[Ian Molton]

On Thu, 14 Aug 2014 17:23:53 +0200
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
> module clock is disabled. Doing so will cause a lock-up.
> 
> When suspending, enable the module clock before disabling the SDHI
> interrupts, and disable the clock again afterwards.

This feels wrong. Why can't interrupts be disabled prior to turning the clock off?

-Ian

> This fixes suspend and resume on r8a7791/koelsch.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index faf0924e71cb..83192420a7e3 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev)
>  {
>  	struct mmc_host *mmc = dev_get_drvdata(dev);
>  	struct tmio_mmc_host *host = mmc_priv(mmc);
> +	struct tmio_mmc_data *pdata = host->pdata;
> +
> +	if (pdata->clk_enable) {
> +		unsigned int f;
> +		pdata->clk_enable(host->pdev, &f);
> +	}
>  
>  	tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
> +
> +	if (pdata->clk_disable)
> +		pdata->clk_disable(host->pdev);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(tmio_mmc_host_suspend);
> -- 
> 1.9.1
> 


-- 
Ian Molton <ian.molton@codethink.co.uk>

Re: [PATCH] mmc: tmio: Fix hang during suspend

[Ulf Hansson]

On 14 August 2014 17:23, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
> module clock is disabled. Doing so will cause a lock-up.
>
> When suspending, enable the module clock before disabling the SDHI
> interrupts, and disable the clock again afterwards.
>
> This fixes suspend and resume on r8a7791/koelsch.

Out of curiosity, are you using CONFIG_PM_RUNTIME to trigger this bug?

Kind regards
Uffe

>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index faf0924e71cb..83192420a7e3 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev)
>  {
>         struct mmc_host *mmc = dev_get_drvdata(dev);
>         struct tmio_mmc_host *host = mmc_priv(mmc);
> +       struct tmio_mmc_data *pdata = host->pdata;
> +
> +       if (pdata->clk_enable) {
> +               unsigned int f;
> +               pdata->clk_enable(host->pdev, &f);
> +       }
>
>         tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
> +
> +       if (pdata->clk_disable)
> +               pdata->clk_disable(host->pdev);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL(tmio_mmc_host_suspend);
> --
> 1.9.1
>

Re: [PATCH] mmc: tmio: Fix hang during suspend

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Aug 15, 2014 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 14 August 2014 17:23, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
>> module clock is disabled. Doing so will cause a lock-up.
>>
>> When suspending, enable the module clock before disabling the SDHI
>> interrupts, and disable the clock again afterwards.
>>
>> This fixes suspend and resume on r8a7791/koelsch.
>
> Out of curiosity, are you using CONFIG_PM_RUNTIME to trigger this bug?

Yes I am:

$ grep CONFIG_PM .config
CONFIG_PM_SLEEP=y
CONFIG_PM_SLEEP_SMP=y
# CONFIG_PM_AUTOSLEEP is not set
# CONFIG_PM_WAKELOCKS is not set
CONFIG_PM_RUNTIME=y
CONFIG_PM=y
CONFIG_PM_DEBUG=y
CONFIG_PM_ADVANCED_DEBUG=y
# CONFIG_PM_TEST_SUSPEND is not set
CONFIG_PM_SLEEP_DEBUG=y
CONFIG_PM_OPP=y
CONFIG_PM_CLK=y
# CONFIG_PMIC_ADP5520 is not set
# CONFIG_PMIC_DA903X is not set
# CONFIG_PM_DEVFREQ is not set
$

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] mmc: tmio: Fix hang during suspend

[Geert Uytterhoeven]

Hi Ian,

On Fri, Aug 15, 2014 at 2:49 PM, Ian Molton <ian.molton@codethink.co.uk> wrote:
> On Thu, 14 Aug 2014 17:23:53 +0200
> Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>
>> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
>> module clock is disabled. Doing so will cause a lock-up.
>>
>> When suspending, enable the module clock before disabling the SDHI
>> interrupts, and disable the clock again afterwards.
>
> This feels wrong. Why can't interrupts be disabled prior to turning the clock off?

The clock is handled by runtime PM. So if SDHI becomes idle, the clock
is turned off.
However, the card insertion interrupt is still enabled.
If all interrupts would be disabled when the clock is turned off, I believe card
insertion can no longer be detected.

BTW, I'm still wondering why tmio_mmc_host_resume() works without
enabling the clock, as it calls tmio_mmc_enable_dma(), which also writes to
an SDHI register (CTL_DMA_ENABLE).

>> This fixes suspend and resume on r8a7791/koelsch.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
>> index faf0924e71cb..83192420a7e3 100644
>> --- a/drivers/mmc/host/tmio_mmc_pio.c
>> +++ b/drivers/mmc/host/tmio_mmc_pio.c
>> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev)
>>  {
>>       struct mmc_host *mmc = dev_get_drvdata(dev);
>>       struct tmio_mmc_host *host = mmc_priv(mmc);
>> +     struct tmio_mmc_data *pdata = host->pdata;
>> +
>> +     if (pdata->clk_enable) {
>> +             unsigned int f;
>> +             pdata->clk_enable(host->pdev, &f);
>> +     }
>>
>>       tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
>> +
>> +     if (pdata->clk_disable)
>> +             pdata->clk_disable(host->pdev);
>> +
>>       return 0;
>>  }
>>  EXPORT_SYMBOL(tmio_mmc_host_suspend);
>> --
>> 1.9.1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] mmc: tmio: Fix hang during suspend

[Ulf Hansson]

On 18 August 2014 14:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ian,
>
> On Fri, Aug 15, 2014 at 2:49 PM, Ian Molton <ian.molton@codethink.co.uk> wrote:
>> On Thu, 14 Aug 2014 17:23:53 +0200
>> Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>
>>> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
>>> module clock is disabled. Doing so will cause a lock-up.
>>>
>>> When suspending, enable the module clock before disabling the SDHI
>>> interrupts, and disable the clock again afterwards.
>>
>> This feels wrong. Why can't interrupts be disabled prior to turning the clock off?
>
> The clock is handled by runtime PM. So if SDHI becomes idle, the clock
> is turned off.
> However, the card insertion interrupt is still enabled.
> If all interrupts would be disabled when the clock is turned off, I believe card
> insertion can no longer be detected.
>
> BTW, I'm still wondering why tmio_mmc_host_resume() works without
> enabling the clock, as it calls tmio_mmc_enable_dma(), which also writes to
> an SDHI register (CTL_DMA_ENABLE).

A while ago, I tried to rework the runtime PM handling of tmio.

http://www.spinics.net/lists/linux-mmc/msg23177.html

The hole set never got merged, due to that we couldn't get them tested
on hardware and there were a lack in review.

We might want to pick them up to see if those may solve our problem. :-)

Kind regards
Uffe

>
>>> This fixes suspend and resume on r8a7791/koelsch.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
>>> index faf0924e71cb..83192420a7e3 100644
>>> --- a/drivers/mmc/host/tmio_mmc_pio.c
>>> +++ b/drivers/mmc/host/tmio_mmc_pio.c
>>> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev)
>>>  {
>>>       struct mmc_host *mmc = dev_get_drvdata(dev);
>>>       struct tmio_mmc_host *host = mmc_priv(mmc);
>>> +     struct tmio_mmc_data *pdata = host->pdata;
>>> +
>>> +     if (pdata->clk_enable) {
>>> +             unsigned int f;
>>> +             pdata->clk_enable(host->pdev, &f);
>>> +     }
>>>
>>>       tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
>>> +
>>> +     if (pdata->clk_disable)
>>> +             pdata->clk_disable(host->pdev);
>>> +
>>>       return 0;
>>>  }
>>>  EXPORT_SYMBOL(tmio_mmc_host_suspend);
>>> --
>>> 1.9.1
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] mmc: tmio: Fix hang during suspend

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Aug 18, 2014 at 3:10 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 18 August 2014 14:36, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Aug 15, 2014 at 2:49 PM, Ian Molton <ian.molton@codethink.co.uk> wrote:
>>> On Thu, 14 Aug 2014 17:23:53 +0200
>>> Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>>> On R-Car Gen 2, the SDHI registers cannot be accessed while the SDHI
>>>> module clock is disabled. Doing so will cause a lock-up.
>>>>
>>>> When suspending, enable the module clock before disabling the SDHI
>>>> interrupts, and disable the clock again afterwards.
>>>
>>> This feels wrong. Why can't interrupts be disabled prior to turning the clock off?
>>
>> The clock is handled by runtime PM. So if SDHI becomes idle, the clock
>> is turned off.
>> However, the card insertion interrupt is still enabled.
>> If all interrupts would be disabled when the clock is turned off, I believe card
>> insertion can no longer be detected.
>>
>> BTW, I'm still wondering why tmio_mmc_host_resume() works without
>> enabling the clock, as it calls tmio_mmc_enable_dma(), which also writes to
>> an SDHI register (CTL_DMA_ENABLE).
>
> A while ago, I tried to rework the runtime PM handling of tmio.
>
> http://www.spinics.net/lists/linux-mmc/msg23177.html
>
> The hole set never got merged, due to that we couldn't get them tested
> on hardware and there were a lack in review.
>
> We might want to pick them up to see if those may solve our problem. :-)

Thanks, I applied patches 4-8 (1-3 did get merged), and tried them on
SDHI1 of r8a7791/koelsch. It still seems to work as before.

However, I still need my patch to fix suspend.
Your "[PATCH 8/8] mmc: tmio: Handle clock gating from runtime PM functions"
manages the SDHI interface clock, while my patch manages the platform-specific
SDHI module clock.

>>>> This fixes suspend and resume on r8a7791/koelsch.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> ---
>>>>  drivers/mmc/host/tmio_mmc_pio.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
>>>> index faf0924e71cb..83192420a7e3 100644
>>>> --- a/drivers/mmc/host/tmio_mmc_pio.c
>>>> +++ b/drivers/mmc/host/tmio_mmc_pio.c
>>>> @@ -1147,8 +1147,18 @@ int tmio_mmc_host_suspend(struct device *dev)
>>>>  {
>>>>       struct mmc_host *mmc = dev_get_drvdata(dev);
>>>>       struct tmio_mmc_host *host = mmc_priv(mmc);
>>>> +     struct tmio_mmc_data *pdata = host->pdata;
>>>> +
>>>> +     if (pdata->clk_enable) {
>>>> +             unsigned int f;
>>>> +             pdata->clk_enable(host->pdev, &f);
>>>> +     }
>>>>
>>>>       tmio_mmc_disable_mmc_irqs(host, TMIO_MASK_ALL);
>>>> +
>>>> +     if (pdata->clk_disable)
>>>> +             pdata->clk_disable(host->pdev);
>>>> +
>>>>       return 0;
>>>>  }
>>>>  EXPORT_SYMBOL(tmio_mmc_host_suspend);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds