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

[Nicolas Pitre <nico@fluxnic.net>]

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@intel.com>
> > Cc: Philip Rakity <prakity@marvell.com>
> > Cc: Aaron Lu <aaron.lu@amd.com>
> > ---

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@laptop.org>   <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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>