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

[Stefan Agner <stefan@agner.ch>]

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);