On Sat, 4 Feb 2012, Chris Ball wrote:
> 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>
> > ---
What if a card asked to be kept powered while the host system was
suspended? Wouldn't this full reset also remove power to the card?
> > 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
>
--
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
