[PATCH] mmc: core: Wait for Vdd to settle on card power off

[PATCH] mmc: core: Wait for Vdd to settle on card power off

[Kyle Roeschley]

The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
lowered to less than 0.5V for a minimum of 1 ms when powering off a
card. Increase our wait to 10 ms so that voltage has time to drain down
to 0.5V and cards can power off correctly.

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
---
 drivers/mmc/core/core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 66c9cf49ad2f..38630246de26 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1679,18 +1679,16 @@ void mmc_power_off(struct mmc_host *host)
 	mmc_set_initial_state(host);
 
 	/*
-	 * Some configurations, such as the 802.11 SDIO card in the OLPC
-	 * XO-1.5, require a short delay after poweroff before the card
-	 * can be successfully turned on again.
+	 * The SD spec requires at least 1 ms with Vdd at less than 0.5 V
+	 * before a card can be re-powered, but we need to wait longer so that
+	 * the voltage has time to drain.
 	 */
-	mmc_delay(1);
+	mmc_delay(10);
 }
 
 void mmc_power_cycle(struct mmc_host *host, u32 ocr)
 {
 	mmc_power_off(host);
-	/* Wait at least 1 ms according to SD spec */
-	mmc_delay(1);
 	mmc_power_up(host, ocr);
 }
 
-- 
2.14.1

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Ulf Hansson]

On 21 September 2017 at 19:47, Kyle Roeschley <kyle.roeschley@ni.com> wrote:
> The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
> lowered to less than 0.5V for a minimum of 1 ms when powering off a
> card. Increase our wait to 10 ms so that voltage has time to drain down
> to 0.5V and cards can power off correctly.
>
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> ---
>  drivers/mmc/core/core.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 66c9cf49ad2f..38630246de26 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1679,18 +1679,16 @@ void mmc_power_off(struct mmc_host *host)
>         mmc_set_initial_state(host);
>
>         /*
> -        * Some configurations, such as the 802.11 SDIO card in the OLPC
> -        * XO-1.5, require a short delay after poweroff before the card
> -        * can be successfully turned on again.
> +        * The SD spec requires at least 1 ms with Vdd at less than 0.5 V
> +        * before a card can be re-powered, but we need to wait longer so that
> +        * the voltage has time to drain.
>          */
> -       mmc_delay(1);
> +       mmc_delay(10);

No, this isn't the proper place of adding more "magic" delays.

Instead, make sure the related ->set_ios() callback in the mmc host
driver deals with this instead. In case it uses an external regulator,
via the regulator API, then this is something that should be
controlled with the definition of the regulator.

>  }
>
>  void mmc_power_cycle(struct mmc_host *host, u32 ocr)
>  {
>         mmc_power_off(host);
> -       /* Wait at least 1 ms according to SD spec */
> -       mmc_delay(1);

Ditto.

>         mmc_power_up(host, ocr);
>  }
>
> --
> 2.14.1
>

Kind regards
Uffe

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Kyle Roeschley]

On Fri, Sep 22, 2017 at 11:38:40AM +0200, Ulf Hansson wrote:
> On 21 September 2017 at 19:47, Kyle Roeschley <kyle.roeschley@ni.com> wrote:
> > The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
> > lowered to less than 0.5V for a minimum of 1 ms when powering off a
> > card. Increase our wait to 10 ms so that voltage has time to drain down
> > to 0.5V and cards can power off correctly.
> >
> > Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> > ---
> >  drivers/mmc/core/core.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 66c9cf49ad2f..38630246de26 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1679,18 +1679,16 @@ void mmc_power_off(struct mmc_host *host)
> >         mmc_set_initial_state(host);
> >
> >         /*
> > -        * Some configurations, such as the 802.11 SDIO card in the OLPC
> > -        * XO-1.5, require a short delay after poweroff before the card
> > -        * can be successfully turned on again.
> > +        * The SD spec requires at least 1 ms with Vdd at less than 0.5 V
> > +        * before a card can be re-powered, but we need to wait longer so that
> > +        * the voltage has time to drain.
> >          */
> > -       mmc_delay(1);
> > +       mmc_delay(10);
> 
> No, this isn't the proper place of adding more "magic" delays.
> 
> Instead, make sure the related ->set_ios() callback in the mmc host
> driver deals with this instead. In case it uses an external regulator,
> via the regulator API, then this is something that should be
> controlled with the definition of the regulator.
> 

Thanks for pointing me in the right direction, I'll reimplement the fix there.

-- 
Kyle Roeschley
Software Engineer
National Instruments

[PATCH] mmc: core: Wait for Vdd to settle on card power off

[Erick Shepherd]

The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
lowered to less than 0.5V for a minimum of 1 ms when powering off a
card. Increase our wait to 15 ms so that voltage has time to drain down
to 0.5V.

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
Signed-off-by: Brad Mouring <brad.mouring@ni.com>
Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
---
 drivers/mmc/host/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f4a7733a8ad2..b15a1f107549 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2415,6 +2415,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	else
 		sdhci_set_power(host, ios->power_mode, ios->vdd);
 
+	if (ios->power_mode == MMC_POWER_OFF)
+		mdelay(15);
+
 	if (host->ops->platform_send_init_74_clocks)
 		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
 
-- 
2.43.0

RE: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Avri Altman]

> Subject: [PATCH] mmc: core: Wait for Vdd to settle on card power off
                                              ^^^^
Should be a host patch?

> 
> The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be lowered to
> less than 0.5V for a minimum of 1 ms when powering off a card. Increase our
> wait to 15 ms so that voltage has time to drain down to 0.5V.
> 
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
Acked-by: Avri Altman <avri.altman@sandisk.com>

> ---
>  drivers/mmc/host/sdhci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> f4a7733a8ad2..b15a1f107549 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2415,6 +2415,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct
> mmc_ios *ios)
>  	else
>  		sdhci_set_power(host, ios->power_mode, ios->vdd);
> 
> +	if (ios->power_mode == MMC_POWER_OFF)
> +		mdelay(15);
> +
>  	if (host->ops->platform_send_init_74_clocks)
>  		host->ops->platform_send_init_74_clocks(host, ios-
> >power_mode);
> 
> --
> 2.43.0
> 

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Adrian Hunter]

On 11/02/25 23:46, Erick Shepherd wrote:
> The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
> lowered to less than 0.5V for a minimum of 1 ms when powering off a
> card. Increase our wait to 15 ms so that voltage has time to drain down
> to 0.5V.

mmc_power_off() has a delay.  So does mmc_power_cycle()

Why does this need to be in sdhci?  Are you experiencing an
issue?

> 
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> Signed-off-by: Erick Shepherd <erick.shepherd@ni.com>
> ---
>  drivers/mmc/host/sdhci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..b15a1f107549 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2415,6 +2415,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  	else
>  		sdhci_set_power(host, ios->power_mode, ios->vdd);
>  
> +	if (ios->power_mode == MMC_POWER_OFF)
> +		mdelay(15);
> +
>  	if (host->ops->platform_send_init_74_clocks)
>  		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>  

RE: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Erick Shepherd]

>> The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
>> lowered to less than 0.5V for a minimum of 1 ms when powering off a
>> card. Increase our wait to 15 ms so that voltage has time to drain down
>> to 0.5V.

> mmc_power_off() has a delay.  So does mmc_power_cycle()

> Why does this need to be in sdhci?  Are you experiencing an
> issue?

Thank you for taking a look at this. The initial change was made in
mmc_power_off() due to an issue we had with some of our devices
requiring more time for the Vdd to drain below 0.5V. Ulf gave us this
feedback on that change:

> No, this isn't the proper place of adding more "magic" delays.

> Instead, make sure the related ->set_ios() callback in the mmc host
> driver deals with this instead. In case it uses an external regulator,
> via the regulator API, then this is something that should be
> controlled with the definition of the regulator.

Should we take a different approach here? 

Regards
Erick

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Adrian Hunter]

On 7/03/25 19:46, Erick Shepherd wrote:
>>> The SD spec version 6.0 section 6.4.1.5 requires that Vdd must be
>>> lowered to less than 0.5V for a minimum of 1 ms when powering off a
>>> card. Increase our wait to 15 ms so that voltage has time to drain down
>>> to 0.5V.
> 
>> mmc_power_off() has a delay.  So does mmc_power_cycle()
> 
>> Why does this need to be in sdhci?  Are you experiencing an
>> issue?
> 
> Thank you for taking a look at this. The initial change was made in
> mmc_power_off() due to an issue we had with some of our devices
> requiring more time for the Vdd to drain below 0.5V. Ulf gave us this
> feedback on that change:
> 
>> No, this isn't the proper place of adding more "magic" delays.
> 
>> Instead, make sure the related ->set_ios() callback in the mmc host
>> driver deals with this instead. In case it uses an external regulator,
>> via the regulator API, then this is something that should be
>> controlled with the definition of the regulator.
> 
> Should we take a different approach here? 

It probably should be dealt with in the ->set_power() callback.
Is it one of the PCI devices in sdhci-pci-core.c?

RE: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Erick Shepherd]

> It probably should be dealt with in the ->set_power() callback.
> Is it one of the PCI devices in sdhci-pci-core.c?

Sure, I can move the delay to sdhci_set_power(). It looks like that
gets called right before the if-statement in the change I proposed
so the behavior should be the same, unless host->ops->set_power is set.

I believe we saw this failure on devices using the Intel Atom E3930
and E3940, which are Apollo Lake. It looks like there is an entry in
sdhci-pci-core.c. Does that change what we should do?

Regards,
Erick

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Adrian Hunter]

On 7/03/25 23:16, Erick Shepherd wrote:
>> It probably should be dealt with in the ->set_power() callback.
>> Is it one of the PCI devices in sdhci-pci-core.c?
> 
> Sure, I can move the delay to sdhci_set_power(). It looks like that
> gets called right before the if-statement in the change I proposed
> so the behavior should be the same, unless host->ops->set_power is set.
> 
> I believe we saw this failure on devices using the Intel Atom E3930
> and E3940, which are Apollo Lake. It looks like there is an entry in
> sdhci-pci-core.c. Does that change what we should do?

What about something like this?

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 1f0bd723f011..0789df732e93 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -610,8 +610,11 @@ static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode,
 
 	sdhci_set_power(host, mode, vdd);
 
-	if (mode == MMC_POWER_OFF)
+	if (mode == MMC_POWER_OFF) {
+		if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD)
+			usleep_range(15000, 17500);
 		return;
+	}
 
 	/*
 	 * Bus power might not enable after D3 -> D0 transition due to the

RE: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Erick Shepherd]

> What about something like this?

> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 1f0bd723f011..0789df732e93 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -610,8 +610,11 @@ static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode,
 
>  	sdhci_set_power(host, mode, vdd);
 
> -	if (mode == MMC_POWER_OFF)
> +	if (mode == MMC_POWER_OFF) {
> +		if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD)
> +			usleep_range(15000, 17500);
>  		return;
> +	}
 
> 	/*
> 	 * Bus power might not enable after D3 -> D0 transition due to the

I talked to one of our digital hardware engineers who worked on this
issue. He believes that the issue is likely affecting more than just 
Apollo Lake devices and recommended keeping the delay for all of our
devices. Could something like this work?

--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2176,6 +2176,9 @@ EXPORT_SYMBOL_GPL(sdhci_set_power_noreg);
 void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 		     unsigned short vdd)
 {
+	if (mode == MMC_POWER_OFF)
+		usleep_range(15000, 17500);
+
 	if (IS_ERR(host->mmc->supply.vmmc))
 		sdhci_set_power_noreg(host, mode, vdd);
 	else
  
Regards,
Erick

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Adrian Hunter]

On 13/03/25 05:35, Erick Shepherd wrote:
>> What about something like this?
> 
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> index 1f0bd723f011..0789df732e93 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -610,8 +610,11 @@ static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode,
>  
>>  	sdhci_set_power(host, mode, vdd);
>  
>> -	if (mode == MMC_POWER_OFF)
>> +	if (mode == MMC_POWER_OFF) {
>> +		if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD)
>> +			usleep_range(15000, 17500);
>>  		return;
>> +	}
>  
>> 	/*
>> 	 * Bus power might not enable after D3 -> D0 transition due to the
> 
> I talked to one of our digital hardware engineers who worked on this
> issue. He believes that the issue is likely affecting more than just 
> Apollo Lake devices and recommended keeping the delay for all of our
> devices. Could something like this work?
> 
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2176,6 +2176,9 @@ EXPORT_SYMBOL_GPL(sdhci_set_power_noreg);
>  void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>  		     unsigned short vdd)
>  {
> +	if (mode == MMC_POWER_OFF)
> +		usleep_range(15000, 17500);
> +
>  	if (IS_ERR(host->mmc->supply.vmmc))
>  		sdhci_set_power_noreg(host, mode, vdd);
>  	else

sdhci is used by a number of drivers (drivers/mmc/host/sdhci*)
that typically use the regulator framework to meet voltage
requirements. So that is not the right place to make changes.

It would be best to put the affected PCI device IDs into
sdhci_intel_set_power() as I showed.

RE: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Erick Shepherd]

> sdhci is used by a number of drivers (drivers/mmc/host/sdhci*)
> that typically use the regulator framework to meet voltage
> requirements. So that is not the right place to make changes.

> It would be best to put the affected PCI device IDs into
> sdhci_intel_set_power() as I showed.

I see, that makes sense. The majority of our devices are using either
Apollo Lake or Bay Trail host controllers. Would it be ok to expand
your solution to include both? I tested the following change on a few
of our devices and confirmed the delay is called. If this looks good I
can submit a V2 of this patch.

--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -610,9 +610,12 @@ static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode,
 
 	sdhci_set_power(host, mode, vdd);
 
-	if (mode == MMC_POWER_OFF)
+	if (mode == MMC_POWER_OFF) {
+		if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD ||
+		    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BYT_SD)
+			usleep_range(15000, 17500);
 		return;
-
+	}
 	/*
 	 * Bus power might not enable after D3 -> D0 transition due to the
 	 * present state not yet having propagated. Retry for up to 2ms.
-- 

Regards,
Erick

Re: [PATCH] mmc: core: Wait for Vdd to settle on card power off

[Adrian Hunter]

On 13/03/25 22:44, Erick Shepherd wrote:
>> sdhci is used by a number of drivers (drivers/mmc/host/sdhci*)
>> that typically use the regulator framework to meet voltage
>> requirements. So that is not the right place to make changes.
> 
>> It would be best to put the affected PCI device IDs into
>> sdhci_intel_set_power() as I showed.
> 
> I see, that makes sense. The majority of our devices are using either
> Apollo Lake or Bay Trail host controllers. Would it be ok to expand
> your solution to include both? I tested the following change on a few
> of our devices and confirmed the delay is called. If this looks good I
> can submit a V2 of this patch.
> 
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -610,9 +610,12 @@ static void sdhci_intel_set_power(struct sdhci_host *host, unsigned char mode,
>  
>  	sdhci_set_power(host, mode, vdd);
>  
> -	if (mode == MMC_POWER_OFF)
> +	if (mode == MMC_POWER_OFF) {
> +		if (slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_APL_SD ||
> +		    slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_BYT_SD)
> +			usleep_range(15000, 17500);
>  		return;
> -
> +	}
>  	/*
>  	 * Bus power might not enable after D3 -> D0 transition due to the
>  	 * present state not yet having propagated. Retry for up to 2ms.

That would be fine