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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <wshfjbdjmoe5hfsmh2rrwxiy67wo4k6i7bzap4hz4n5j22uiol@xz7wfxaw4bve>
Date: Mon, 10 Nov 2025 08:39:20 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Sarthak Garg <sarthak.garg@....qualcomm.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>, 
	Ulf Hansson <ulf.hansson@...aro.org>, linux-mmc@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, quic_nguyenb@...cinc.com, quic_rampraka@...cinc.com, 
	quic_pragalla@...cinc.com, quic_sayalil@...cinc.com, quic_nitirawa@...cinc.com, 
	quic_bhaskarv@...cinc.com, kernel@....qualcomm.com
Subject: Re: [PATCH V1] mmc: sdhci-msm: Avoid early clock doubling during
 HS400 transition

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