Re: [PATCH 2/2] mmc: sdhci: always reset all during resume

[Chris Ball <cjb@laptop.org>]

Hi, adding Nico for review,

On Mon, Jan 30 2012, Adrian Hunter wrote:
> During suspend the host controller may or may not be powered off.
> In order to get the same result either way, always perform a
> software "reset all" when resuming.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Philip Rakity <prakity@marvell.com>
> Cc: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/mmc/host/sdhci.c |   29 +++++++++++------------------
>  1 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..ef2434c 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -219,31 +219,20 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
>  	}
>  }
>  
> -static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios);
> -
> -static void sdhci_init(struct sdhci_host *host, int soft)
> +static void sdhci_init(struct sdhci_host *host)
>  {
> -	if (soft)
> -		sdhci_reset(host, SDHCI_RESET_CMD|SDHCI_RESET_DATA);
> -	else
> -		sdhci_reset(host, SDHCI_RESET_ALL);
> +	sdhci_reset(host, SDHCI_RESET_ALL);
>  
>  	sdhci_clear_set_irqs(host, SDHCI_INT_ALL_MASK,
>  		SDHCI_INT_BUS_POWER | SDHCI_INT_DATA_END_BIT |
>  		SDHCI_INT_DATA_CRC | SDHCI_INT_DATA_TIMEOUT | SDHCI_INT_INDEX |
>  		SDHCI_INT_END_BIT | SDHCI_INT_CRC | SDHCI_INT_TIMEOUT |
>  		SDHCI_INT_DATA_END | SDHCI_INT_RESPONSE);
> -
> -	if (soft) {
> -		/* force clock reconfiguration */
> -		host->clock = 0;
> -		sdhci_set_ios(host->mmc, &host->mmc->ios);
> -	}
>  }
>  
>  static void sdhci_reinit(struct sdhci_host *host)
>  {
> -	sdhci_init(host, 0);
> +	sdhci_init(host);
>  	sdhci_enable_card_detection(host);
>  }
>  
> @@ -2423,8 +2412,12 @@ int sdhci_resume_host(struct sdhci_host *host)
>  	if (ret)
>  		return ret;
>  
> -	sdhci_init(host, (host->mmc->pm_flags & MMC_PM_KEEP_POWER));
> -	mmiowb();
> +	sdhci_init(host);
> +
> +	/* Force clock and power re-program */
> +	host->pwr = 0;
> +	host->clock = 0;
> +	sdhci_do_set_ios(host, &host->mmc->ios);
>  
>  	ret = mmc_resume_host(host->mmc);
>  	sdhci_enable_card_detection(host);
> @@ -2500,7 +2493,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host)
>  			host->ops->enable_dma(host);
>  	}
>  
> -	sdhci_init(host, 0);
> +	sdhci_init(host);
>  
>  	/* Force clock and power re-program */
>  	host->pwr = 0;
> @@ -2980,7 +2973,7 @@ int sdhci_add_host(struct sdhci_host *host)
>  		host->vmmc = NULL;
>  	}
>  
> -	sdhci_init(host, 0);
> +	sdhci_init(host);
>  
>  #ifdef CONFIG_MMC_DEBUG
>  	sdhci_dumpregs(host);

This patch doesn't look correct to me -- yes, the hardware might have
lost power during suspend, but that's what the common-case of:

        sdhci_init(host, soft=0);

is for.  soft=1 is supposed to indicate that the power stayed up via
MMC_PM_KEEP_POWER, but you're changing that by setting ->pwr = 0 in the
soft=1 path.  If I'm using soft=1 because my controller has a wifi SDIO
card on it, I don't want you to reprogram the power to it on resume.
Does that make sense?

(Doing a RESET_ALL might also destroy some wanted soft-state.)

Thanks,

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