[PATCH] mmc: sdhci-of-dwcmshc: Disable clock before DLL configuration

[PATCH] mmc: sdhci-of-dwcmshc: Disable clock before DLL configuration

[Shawn Lin]

According to the ASIC design recommendations, the clock must be
disabled before operating the DLL to prevent glitches that could
affect the internal digital logic. In extreme cases, failing to
do so may cause the controller to malfunction completely.

Adds a step to disable the clock before DLL configuration and
re-enables it at the end.

Fixes: 08f3dff799d4 ("mmc: sdhci-of-dwcmshc: add rockchip platform support")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
This is bascially a code sync with the downstream vendor kernel which was been
done this way and tested for some years to confirm it could fix the issues in
all corner cases.

 drivers/mmc/host/sdhci-of-dwcmshc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 6139516..e3ae334 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -783,12 +783,15 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
 	extra |= BIT(4);
 	sdhci_writel(host, extra, reg);
 
+	/* Disable clock while config DLL */
+	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+
 	if (clock <= 52000000) {
 		if (host->mmc->ios.timing == MMC_TIMING_MMC_HS200 ||
 		    host->mmc->ios.timing == MMC_TIMING_MMC_HS400) {
 			dev_err(mmc_dev(host->mmc),
 				"Can't reduce the clock below 52MHz in HS200/HS400 mode");
-			return;
+			goto enable_clk;
 		}
 
 		/*
@@ -808,7 +811,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
 			DLL_STRBIN_DELAY_NUM_SEL |
 			DLL_STRBIN_DELAY_NUM_DEFAULT << DLL_STRBIN_DELAY_NUM_OFFSET;
 		sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
-		return;
+		goto enable_clk;
 	}
 
 	/* Reset DLL */
@@ -835,7 +838,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
 				 500 * USEC_PER_MSEC);
 	if (err) {
 		dev_err(mmc_dev(host->mmc), "DLL lock timeout!\n");
-		return;
+		goto enable_clk;
 	}
 
 	extra = 0x1 << 16 | /* tune clock stop en */
@@ -868,6 +871,9 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
 		DLL_STRBIN_TAPNUM_DEFAULT |
 		DLL_STRBIN_TAPNUM_FROM_SW;
 	sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
+
+enable_clk:
+	sdhci_enable_clk(host, 0);
 }
 
 static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
-- 
2.7.4

Re: [PATCH] mmc: sdhci-of-dwcmshc: Disable clock before DLL configuration

[Adrian Hunter]

On 01/04/2026 06:39, Shawn Lin wrote:
> According to the ASIC design recommendations, the clock must be
> disabled before operating the DLL to prevent glitches that could
> affect the internal digital logic. In extreme cases, failing to
> do so may cause the controller to malfunction completely.
> 
> Adds a step to disable the clock before DLL configuration and
> re-enables it at the end.
> 
> Fixes: 08f3dff799d4 ("mmc: sdhci-of-dwcmshc: add rockchip platform support")
> Cc: <Stable@vger.kernel.org>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> This is bascially a code sync with the downstream vendor kernel which was been
> done this way and tested for some years to confirm it could fix the issues in
> all corner cases.
> 
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 6139516..e3ae334 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -783,12 +783,15 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>  	extra |= BIT(4);
>  	sdhci_writel(host, extra, reg);
>  
> +	/* Disable clock while config DLL */
> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
>  	if (clock <= 52000000) {
>  		if (host->mmc->ios.timing == MMC_TIMING_MMC_HS200 ||
>  		    host->mmc->ios.timing == MMC_TIMING_MMC_HS400) {
>  			dev_err(mmc_dev(host->mmc),
>  				"Can't reduce the clock below 52MHz in HS200/HS400 mode");
> -			return;
> +			goto enable_clk;
>  		}
>  
>  		/*
> @@ -808,7 +811,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>  			DLL_STRBIN_DELAY_NUM_SEL |
>  			DLL_STRBIN_DELAY_NUM_DEFAULT << DLL_STRBIN_DELAY_NUM_OFFSET;
>  		sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
> -		return;
> +		goto enable_clk;
>  	}
>  
>  	/* Reset DLL */
> @@ -835,7 +838,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>  				 500 * USEC_PER_MSEC);
>  	if (err) {
>  		dev_err(mmc_dev(host->mmc), "DLL lock timeout!\n");
> -		return;
> +		goto enable_clk;
>  	}
>  
>  	extra = 0x1 << 16 | /* tune clock stop en */
> @@ -868,6 +871,9 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>  		DLL_STRBIN_TAPNUM_DEFAULT |
>  		DLL_STRBIN_TAPNUM_FROM_SW;
>  	sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
> +
> +enable_clk:
> +	sdhci_enable_clk(host, 0);

Should this be 0?  If so, needs some explanation.

>  }
>  
>  static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)

Re: [PATCH] mmc: sdhci-of-dwcmshc: Disable clock before DLL configuration

[Shawn Lin]

在 2026/04/07 星期二 14:38, Adrian Hunter 写道:
> On 01/04/2026 06:39, Shawn Lin wrote:
>> According to the ASIC design recommendations, the clock must be
>> disabled before operating the DLL to prevent glitches that could
>> affect the internal digital logic. In extreme cases, failing to
>> do so may cause the controller to malfunction completely.
>>
>> Adds a step to disable the clock before DLL configuration and
>> re-enables it at the end.
>>
>> Fixes: 08f3dff799d4 ("mmc: sdhci-of-dwcmshc: add rockchip platform support")
>> Cc: <Stable@vger.kernel.org>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>> This is bascially a code sync with the downstream vendor kernel which was been
>> done this way and tested for some years to confirm it could fix the issues in
>> all corner cases.
>>
>>   drivers/mmc/host/sdhci-of-dwcmshc.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 6139516..e3ae334 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -783,12 +783,15 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>   	extra |= BIT(4);
>>   	sdhci_writel(host, extra, reg);
>>   
>> +	/* Disable clock while config DLL */
>> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>> +
>>   	if (clock <= 52000000) {
>>   		if (host->mmc->ios.timing == MMC_TIMING_MMC_HS200 ||
>>   		    host->mmc->ios.timing == MMC_TIMING_MMC_HS400) {
>>   			dev_err(mmc_dev(host->mmc),
>>   				"Can't reduce the clock below 52MHz in HS200/HS400 mode");
>> -			return;
>> +			goto enable_clk;
>>   		}
>>   
>>   		/*
>> @@ -808,7 +811,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>   			DLL_STRBIN_DELAY_NUM_SEL |
>>   			DLL_STRBIN_DELAY_NUM_DEFAULT << DLL_STRBIN_DELAY_NUM_OFFSET;
>>   		sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>> -		return;
>> +		goto enable_clk;
>>   	}
>>   
>>   	/* Reset DLL */
>> @@ -835,7 +838,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>   				 500 * USEC_PER_MSEC);
>>   	if (err) {
>>   		dev_err(mmc_dev(host->mmc), "DLL lock timeout!\n");
>> -		return;
>> +		goto enable_clk;
>>   	}
>>   
>>   	extra = 0x1 << 16 | /* tune clock stop en */
>> @@ -868,6 +871,9 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>   		DLL_STRBIN_TAPNUM_DEFAULT |
>>   		DLL_STRBIN_TAPNUM_FROM_SW;
>>   	sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>> +
>> +enable_clk:
>> +	sdhci_enable_clk(host, 0);
> 
> Should this be 0?  If so, needs some explanation.

Yes, passing 0 is intentional for the Rockchip platform.
This controller on Rockchip has SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN
set, indicating that the base clock capability reporting is unreliable.
More importantly, the sdclk frequency select bits in the
SDHCI_CLOCK_CONTROL register are actually non-functional on this 
hardware. The sdclk frequency is instead set via clk_set_rate()for all
modes. From this point, all of the sdhci_set_clock() calls and in
dwcmshc_rk3568_set_clock() are also served as enabling and disabling
clk only.

Therefore, sdhci_enable_clk(host, 0) is simply re-enabling the
clock without modifying the frequency selection bits, which aligns with
the hardware's actual behavior.

Technically speaking, we could save the previously calculated sdclk
value and pass it to sdhci_enable_clk(). However, since the frequency
select bits are ignored by the hardware, passing 0 is safe and
functionally equivalent. I could add a comment for just this line change
or would you like me to add a comment for the whole sdclk stuff in
dwcmshc_rk3568_set_clock()?


> 
>>   }
>>   
>>   static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> 
> 

Re: [PATCH] mmc: sdhci-of-dwcmshc: Disable clock before DLL configuration

[Adrian Hunter]

On 08/04/2026 05:18, Shawn Lin wrote:
> 在 2026/04/07 星期二 14:38, Adrian Hunter 写道:
>> On 01/04/2026 06:39, Shawn Lin wrote:
>>> According to the ASIC design recommendations, the clock must be
>>> disabled before operating the DLL to prevent glitches that could
>>> affect the internal digital logic. In extreme cases, failing to
>>> do so may cause the controller to malfunction completely.
>>>
>>> Adds a step to disable the clock before DLL configuration and
>>> re-enables it at the end.
>>>
>>> Fixes: 08f3dff799d4 ("mmc: sdhci-of-dwcmshc: add rockchip platform support")
>>> Cc: <Stable@vger.kernel.org>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>> ---
>>> This is bascially a code sync with the downstream vendor kernel which was been
>>> done this way and tested for some years to confirm it could fix the issues in
>>> all corner cases.
>>>
>>>   drivers/mmc/host/sdhci-of-dwcmshc.c | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> index 6139516..e3ae334 100644
>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> @@ -783,12 +783,15 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>>       extra |= BIT(4);
>>>       sdhci_writel(host, extra, reg);
>>>   +    /* Disable clock while config DLL */
>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>> +
>>>       if (clock <= 52000000) {
>>>           if (host->mmc->ios.timing == MMC_TIMING_MMC_HS200 ||
>>>               host->mmc->ios.timing == MMC_TIMING_MMC_HS400) {
>>>               dev_err(mmc_dev(host->mmc),
>>>                   "Can't reduce the clock below 52MHz in HS200/HS400 mode");
>>> -            return;
>>> +            goto enable_clk;
>>>           }
>>>             /*
>>> @@ -808,7 +811,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>>               DLL_STRBIN_DELAY_NUM_SEL |
>>>               DLL_STRBIN_DELAY_NUM_DEFAULT << DLL_STRBIN_DELAY_NUM_OFFSET;
>>>           sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>>> -        return;
>>> +        goto enable_clk;
>>>       }
>>>         /* Reset DLL */
>>> @@ -835,7 +838,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>>                    500 * USEC_PER_MSEC);
>>>       if (err) {
>>>           dev_err(mmc_dev(host->mmc), "DLL lock timeout!\n");
>>> -        return;
>>> +        goto enable_clk;
>>>       }
>>>         extra = 0x1 << 16 | /* tune clock stop en */
>>> @@ -868,6 +871,9 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock
>>>           DLL_STRBIN_TAPNUM_DEFAULT |
>>>           DLL_STRBIN_TAPNUM_FROM_SW;
>>>       sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>>> +
>>> +enable_clk:
>>> +    sdhci_enable_clk(host, 0);
>>
>> Should this be 0?  If so, needs some explanation.
> 
> Yes, passing 0 is intentional for the Rockchip platform.
> This controller on Rockchip has SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN
> set, indicating that the base clock capability reporting is unreliable.
> More importantly, the sdclk frequency select bits in the
> SDHCI_CLOCK_CONTROL register are actually non-functional on this hardware. The sdclk frequency is instead set via clk_set_rate()for all
> modes. From this point, all of the sdhci_set_clock() calls and in
> dwcmshc_rk3568_set_clock() are also served as enabling and disabling
> clk only.
> 
> Therefore, sdhci_enable_clk(host, 0) is simply re-enabling the
> clock without modifying the frequency selection bits, which aligns with
> the hardware's actual behavior.
> 
> Technically speaking, we could save the previously calculated sdclk
> value and pass it to sdhci_enable_clk(). However, since the frequency
> select bits are ignored by the hardware, passing 0 is safe and
> functionally equivalent. I could add a comment for just this line change

Yes please

> or would you like me to add a comment for the whole sdclk stuff in
> dwcmshc_rk3568_set_clock()?
> 
> 
>>
>>>   }
>>>     static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>>
>>