[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 at vger.kernel.org>
>>> Signed-off-by: Shawn Lin <shawn.lin at 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)
>>
>>