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

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

Posted by Erick Shepherd 10 months, 1 week ago

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

Posted by Adrian Hunter 9 months, 2 weeks ago

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

Posted by Erick Shepherd 9 months, 2 weeks ago

>> 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

Posted by Adrian Hunter 9 months, 2 weeks ago

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

Posted by Erick Shepherd 9 months, 2 weeks ago

> 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

Posted by Adrian Hunter 9 months, 1 week ago

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

Posted by Erick Shepherd 9 months, 1 week ago

> 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

Posted by Adrian Hunter 9 months, 1 week ago

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

Posted by Erick Shepherd 9 months, 1 week ago

> 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

Posted by Adrian Hunter 9 months, 1 week ago

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