[PATCH 0/2] mmc: sdhci-pci: fixes for Medfield SDIO suspend / resume

[PATCH 0/2] mmc: sdhci-pci: fixes for Medfield SDIO suspend / resume

[Adrian Hunter]

Hi

Here are a couple of fixes for Medfield SDIO suspend / resume.


Adrian Hunter (2):
      mmc: sdhci-pci: set Medfield SDIO as non-removable
      mmc: sdhci: always reset all during resume

 drivers/mmc/host/sdhci-pci.c |    2 +-
 drivers/mmc/host/sdhci.c     |   29 +++++++++++------------------
 2 files changed, 12 insertions(+), 19 deletions(-)


Regards
Adrian

[PATCH 1/2] mmc: sdhci-pci: set Medfield SDIO as non-removable

[Adrian Hunter]

Set Medfield SDIO as non-removable to avoid un-necessary
card detect activity.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 7165e6a..6ebdc40 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -250,7 +250,7 @@ static int mfd_emmc_probe_slot(struct sdhci_pci_slot *slot)
 
 static int mfd_sdio_probe_slot(struct sdhci_pci_slot *slot)
 {
-	slot->host->mmc->caps |= MMC_CAP_POWER_OFF_CARD;
+	slot->host->mmc->caps |= MMC_CAP_POWER_OFF_CARD | MMC_CAP_NONREMOVABLE;
 	return 0;
 }
 
-- 
1.7.6.4

Re: [PATCH 1/2] mmc: sdhci-pci: set Medfield SDIO as non-removable

[Chris Ball]

Hi,

On Mon, Jan 30 2012, Adrian Hunter wrote:
> Set Medfield SDIO as non-removable to avoid un-necessary
> card detect activity.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 7165e6a..6ebdc40 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -250,7 +250,7 @@ static int mfd_emmc_probe_slot(struct sdhci_pci_slot *slot)
>  
>  static int mfd_sdio_probe_slot(struct sdhci_pci_slot *slot)
>  {
> -	slot->host->mmc->caps |= MMC_CAP_POWER_OFF_CARD;
> +	slot->host->mmc->caps |= MMC_CAP_POWER_OFF_CARD | MMC_CAP_NONREMOVABLE;
>  	return 0;
>  }

I've pushed this to mmc-next for 3.3.  Thanks,

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

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

[Adrian Hunter]

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

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

[Aaron Lu]

On Mon, Jan 30, 2012 at 02:27:19PM +0200, 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>

Acked-by: 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);
> -- 
> 1.7.6.4
> 
> 

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

[Chris Ball]

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

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

[Nicolas Pitre]

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
> 

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

[Adrian Hunter]

On 05/02/12 03:09, Chris Ball wrote:
> 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?

Yes, I will send a different patch (to handle the "card is on, but the host
controller is off" case)