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@xxxxxxxxx>
> Cc: Philip Rakity <prakity@xxxxxxxxxxx>
> Cc: Aaron Lu <aaron.lu@xxxxxxx>
> ---
> 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@xxxxxxxxxx> <http://printf.net/>
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
