[v1] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition

[PATCH V1] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition

Posted by Sarthak Garg 1 month, 1 week ago

According to the hardware programming guide, the clock frequency must
remain below 52MHz during the transition to HS400 mode.

However,in the current implementation, the timing is set to HS400 (a
DDR mode) before adjusting the clock. This causes the clock to double
prematurely to 104MHz during the transition phase, violating the
specification and potentially resulting in CRC errors or CMD timeouts.

This change ensures that clock doubling is avoided during intermediate
transitions and is applied only when the card requires a 200MHz clock
for HS400 operation.

Signed-off-by: Sarthak Garg <sarthak.garg@oss.qualcomm.com>
---
 drivers/mmc/host/sdhci-msm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 4e5edbf2fc9b..eca6a09a4547 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -355,7 +355,8 @@ static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host)
 	 */
 	if (ios.timing == MMC_TIMING_UHS_DDR50 ||
 	    ios.timing == MMC_TIMING_MMC_DDR52 ||
-	    ios.timing == MMC_TIMING_MMC_HS400 ||
+	    (ios.timing == MMC_TIMING_MMC_HS400 &&
+	    ios.clock == MMC_HS200_MAX_DTR) ||
 	    host->flags & SDHCI_HS400_TUNING)
 		return 2;
 	return 1;
-- 
2.34.1

Re: [PATCH V1] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition

Posted by Bjorn Andersson 1 month, 1 week ago

On Mon, Nov 10, 2025 at 12:08:01PM +0530, Sarthak Garg wrote:
> According to the hardware programming guide, the clock frequency must
> remain below 52MHz during the transition to HS400 mode.
> 
> However,in the current implementation, the timing is set to HS400 (a
> DDR mode) before adjusting the clock. This causes the clock to double
> prematurely to 104MHz during the transition phase, violating the
> specification and potentially resulting in CRC errors or CMD timeouts.
> 
> This change ensures that clock doubling is avoided during intermediate
> transitions and is applied only when the card requires a 200MHz clock
> for HS400 operation.
> 
> Signed-off-by: Sarthak Garg <sarthak.garg@oss.qualcomm.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 4e5edbf2fc9b..eca6a09a4547 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -355,7 +355,8 @@ static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host)
>  	 */
>  	if (ios.timing == MMC_TIMING_UHS_DDR50 ||
>  	    ios.timing == MMC_TIMING_MMC_DDR52 ||
> -	    ios.timing == MMC_TIMING_MMC_HS400 ||
> +	    (ios.timing == MMC_TIMING_MMC_HS400 &&
> +	    ios.clock == MMC_HS200_MAX_DTR) ||

It's a bit ugly that sdhci_msm_execute_tuning() passes ios.clock as an
argument to msm_set_clock_rate_for_bus_mode(), which then calls
msm_get_clock_mult_for_bus_mode() where you reach back into ios.clock.

In fact, having msm_get_clock_mult_for_bus_mode() reach into
host->mmc->ios to get ios.timing, seems a violation of the original
intent of the prototype.

How about cleaning this up and passing "timing" as an argument to
msm_set_clock_rate_for_bus_mode(), and then pass host, clock, and timing
to msm_get_clock_mult_for_bus_mode()?

That way we avoid this mix of passing parameters to the functions in
both arguments and in state at the same time.

Regards,
Bjorn

>  	    host->flags & SDHCI_HS400_TUNING)
>  		return 2;
>  	return 1;
> -- 
> 2.34.1
> 
>

Re: [PATCH V1] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition

Posted by Sarthak Garg 1 month ago

On 11/10/2025 8:09 PM, Bjorn Andersson wrote:
> On Mon, Nov 10, 2025 at 12:08:01PM +0530, Sarthak Garg wrote:
>> According to the hardware programming guide, the clock frequency must
>> remain below 52MHz during the transition to HS400 mode.
>>
>> However,in the current implementation, the timing is set to HS400 (a
>> DDR mode) before adjusting the clock. This causes the clock to double
>> prematurely to 104MHz during the transition phase, violating the
>> specification and potentially resulting in CRC errors or CMD timeouts.
>>
>> This change ensures that clock doubling is avoided during intermediate
>> transitions and is applied only when the card requires a 200MHz clock
>> for HS400 operation.
>>
>> Signed-off-by: Sarthak Garg <sarthak.garg@oss.qualcomm.com>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4e5edbf2fc9b..eca6a09a4547 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -355,7 +355,8 @@ static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host)
>>   	 */
>>   	if (ios.timing == MMC_TIMING_UHS_DDR50 ||
>>   	    ios.timing == MMC_TIMING_MMC_DDR52 ||
>> -	    ios.timing == MMC_TIMING_MMC_HS400 ||
>> +	    (ios.timing == MMC_TIMING_MMC_HS400 &&
>> +	    ios.clock == MMC_HS200_MAX_DTR) ||
> It's a bit ugly that sdhci_msm_execute_tuning() passes ios.clock as an
> argument to msm_set_clock_rate_for_bus_mode(), which then calls
> msm_get_clock_mult_for_bus_mode() where you reach back into ios.clock.
>
> In fact, having msm_get_clock_mult_for_bus_mode() reach into
> host->mmc->ios to get ios.timing, seems a violation of the original
> intent of the prototype.
>
>
> How about cleaning this up and passing "timing" as an argument to
> msm_set_clock_rate_for_bus_mode(), and then pass host, clock, and timing
> to msm_get_clock_mult_for_bus_mode()?
>
> That way we avoid this mix of passing parameters to the functions in
> both arguments and in state at the same time.
>
> Regards,
> Bjorn


Sure will update in V2.

Regards,
Sarthak


>>   	    host->flags & SDHCI_HS400_TUNING)
>>   		return 2;
>>   	return 1;
>> -- 
>> 2.34.1
>>
>>