mmc: sdhci: Disable SD card clock before changing parameters

[PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Erick Shepherd 12 months ago

Per the SD Host Controller Simplified Specification v4.20 §3.2.3, change
the SD card clock parameters only after first disabling the external card
clock. Doing this fixes a spurious clock pulse on Baytrail and Apollo Lake
SD controllers which otherwise breaks voltage switching with a specific
Swissbit SD card.

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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f4a7733a8ad2..5f91b44891f9 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2065,10 +2065,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	host->mmc->actual_clock = 0;
 
-	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (clk & SDHCI_CLOCK_CARD_EN)
+		sdhci_writew(host, clk & ~SDHCI_CLOCK_CARD_EN,
+			SDHCI_CLOCK_CONTROL);
 
-	if (clock == 0)
+	if (clock == 0) {
+		sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 		return;
+	}
 
 	clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
 	sdhci_enable_clk(host, clk);
-- 
2.43.0

Re: [PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Jonathan Liu 7 months, 3 weeks ago

Hi Erick,

On Tue, 11 Feb 2025 at 15:46, Erick Shepherd <erick.shepherd@ni.com> wrote:
>
> Per the SD Host Controller Simplified Specification v4.20 §3.2.3, change
> the SD card clock parameters only after first disabling the external card
> clock. Doing this fixes a spurious clock pulse on Baytrail and Apollo Lake
> SD controllers which otherwise breaks voltage switching with a specific
> Swissbit SD card.
>
> 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 | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..5f91b44891f9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2065,10 +2065,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>
>         host->mmc->actual_clock = 0;
>
> -       sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +       clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if (clk & SDHCI_CLOCK_CARD_EN)
> +               sdhci_writew(host, clk & ~SDHCI_CLOCK_CARD_EN,
> +                       SDHCI_CLOCK_CONTROL);
>
> -       if (clock == 0)
> +       if (clock == 0) {
> +               sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>                 return;
> +       }
>
>         clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>         sdhci_enable_clk(host, clk);
> --
> 2.43.0

This commit is causing eMMC to sometimes fail to detect on my RK3399
ARM64 board after updating from v5.10.237 to v5.10.238.
Any ideas?

Regards,
Jonathan

Re: [PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Adrian Hunter 7 months, 2 weeks ago

On 19/06/2025 06:29, Jonathan Liu wrote:
> Hi Erick,
> 
> On Tue, 11 Feb 2025 at 15:46, Erick Shepherd <erick.shepherd@ni.com> wrote:
>>
>> Per the SD Host Controller Simplified Specification v4.20 §3.2.3, change
>> the SD card clock parameters only after first disabling the external card
>> clock. Doing this fixes a spurious clock pulse on Baytrail and Apollo Lake
>> SD controllers which otherwise breaks voltage switching with a specific
>> Swissbit SD card.
>>
>> 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 | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index f4a7733a8ad2..5f91b44891f9 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2065,10 +2065,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>>
>>         host->mmc->actual_clock = 0;
>>
>> -       sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>> +       clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> +       if (clk & SDHCI_CLOCK_CARD_EN)
>> +               sdhci_writew(host, clk & ~SDHCI_CLOCK_CARD_EN,
>> +                       SDHCI_CLOCK_CONTROL);
>>
>> -       if (clock == 0)
>> +       if (clock == 0) {
>> +               sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>                 return;
>> +       }
>>
>>         clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>>         sdhci_enable_clk(host, clk);
>> --
>> 2.43.0
> 
> This commit is causing eMMC to sometimes fail to detect on my RK3399
> ARM64 board after updating from v5.10.237 to v5.10.238.
> Any ideas?

Looks like this patch will get reverted.

After that, would something like the following work instead?

diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index dd084c8750f9..ad95218fe3b3 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -679,8 +679,19 @@ static int intel_start_signal_voltage_switch(struct mmc_host *mmc,
 	return 0;
 }
 
+static void sdhci_intel_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	u16 clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+
+	/* Stop card clock separately to avoid glitches on clock line */
+	if (clk & SDHCI_CLOCK_CARD_EN)
+		sdhci_writew(host, clk & ~SDHCI_CLOCK_CARD_EN, SDHCI_CLOCK_CONTROL);
+
+	sdhci_set_clock(host, clock);
+}
+
 static const struct sdhci_ops sdhci_intel_byt_ops = {
-	.set_clock		= sdhci_set_clock,
+	.set_clock		= sdhci_intel_set_clock,
 	.set_power		= sdhci_intel_set_power,
 	.enable_dma		= sdhci_pci_enable_dma,
 	.set_bus_width		= sdhci_set_bus_width,
@@ -690,7 +701,7 @@ static const struct sdhci_ops sdhci_intel_byt_ops = {
 };
 
 static const struct sdhci_ops sdhci_intel_glk_ops = {
-	.set_clock		= sdhci_set_clock,
+	.set_clock		= sdhci_intel_set_clock,
 	.set_power		= sdhci_intel_set_power,
 	.enable_dma		= sdhci_pci_enable_dma,
 	.set_bus_width		= sdhci_set_bus_width,

Re: [PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Erick Shepherd 6 months, 2 weeks ago

Hi Adrian,

I believe this fix would work since it looks like it keeps the
functionality of the previous change but only for intel SD
controllers. I also tested your proposed change on one of our devices
with a Baytrail SD controller and a Swissbit SD card and did not run
into any issues.

Regards,
Erick

Re: [PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Adrian Hunter 6 months, 2 weeks ago

On 23/07/2025 00:29, Erick Shepherd wrote:
> Hi Adrian,
> 
> I believe this fix would work since it looks like it keeps the
> functionality of the previous change but only for intel SD
> controllers. I also tested your proposed change on one of our devices
> with a Baytrail SD controller and a Swissbit SD card and did not run
> into any issues.

If you are OK with it, please submit it as a patch

Re: [PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Ulf Hansson 11 months ago

On Tue, 11 Feb 2025 at 22:47, Erick Shepherd <erick.shepherd@ni.com> wrote:
>
> Per the SD Host Controller Simplified Specification v4.20 §3.2.3, change
> the SD card clock parameters only after first disabling the external card
> clock. Doing this fixes a spurious clock pulse on Baytrail and Apollo Lake
> SD controllers which otherwise breaks voltage switching with a specific
> Swissbit SD card.
>
> 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>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..5f91b44891f9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2065,10 +2065,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>
>         host->mmc->actual_clock = 0;
>
> -       sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +       clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if (clk & SDHCI_CLOCK_CARD_EN)
> +               sdhci_writew(host, clk & ~SDHCI_CLOCK_CARD_EN,
> +                       SDHCI_CLOCK_CONTROL);
>
> -       if (clock == 0)
> +       if (clock == 0) {
> +               sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>                 return;
> +       }
>
>         clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>         sdhci_enable_clk(host, clk);
> --
> 2.43.0
>

Re: [PATCH] mmc: sdhci: Disable SD card clock before changing parameters

Posted by Adrian Hunter 11 months, 1 week ago

On 11/02/25 23:46, Erick Shepherd wrote:
> Per the SD Host Controller Simplified Specification v4.20 §3.2.3, change
> the SD card clock parameters only after first disabling the external card
> clock. Doing this fixes a spurious clock pulse on Baytrail and Apollo Lake
> SD controllers which otherwise breaks voltage switching with a specific
> Swissbit SD card.
> 
> 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: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f4a7733a8ad2..5f91b44891f9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2065,10 +2065,15 @@ void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	host->mmc->actual_clock = 0;
>  
> -	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (clk & SDHCI_CLOCK_CARD_EN)
> +		sdhci_writew(host, clk & ~SDHCI_CLOCK_CARD_EN,
> +			SDHCI_CLOCK_CONTROL);
>  
> -	if (clock == 0)
> +	if (clock == 0) {
> +		sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>  		return;
> +	}
>  
>  	clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>  	sdhci_enable_clk(host, clk);