linux-kernel - Re: [PATCH V2] 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: <e4t2tutkygmka6ynytztjy47mey3trwekyyzxx7dzyqnb3xmqq@3gk5zgj5kvqg>
Date: Fri, 14 Nov 2025 08:58:15 -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-arm-msm@...r.kernel.org, linux-mmc@...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 V2] mmc: sdhci-msm: Avoid early clock doubling during
 HS400 transition

On Fri, Nov 14, 2025 at 01:58:24PM +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>

Thank you for cleaning that up.

Reviewed-by: Bjorn Andersson <andersson@...nel.org>

Regards,
Bjorn

> ---
>  Changes from v1:
>  As per Bjorn Andersson's comment :
>  - Pass "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() to
>  align with the original intent of the prototype.
> ---
>  drivers/mmc/host/sdhci-msm.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 4e5edbf2fc9b..3b85233131b3 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -344,41 +344,43 @@ static void sdhci_msm_v5_variant_writel_relaxed(u32 val,
>  	writel_relaxed(val, host->ioaddr + offset);
>  }
>  
> -static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host)
> +static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host,
> +						    unsigned int clock,
> +						    unsigned int timing)
>  {
> -	struct mmc_ios ios = host->mmc->ios;
>  	/*
>  	 * The SDHC requires internal clock frequency to be double the
>  	 * actual clock that will be set for DDR mode. The controller
>  	 * uses the faster clock(100/400MHz) for some of its parts and
>  	 * send the actual required clock (50/200MHz) to the card.
>  	 */
> -	if (ios.timing == MMC_TIMING_UHS_DDR50 ||
> -	    ios.timing == MMC_TIMING_MMC_DDR52 ||
> -	    ios.timing == MMC_TIMING_MMC_HS400 ||
> +	if (timing == MMC_TIMING_UHS_DDR50 ||
> +	    timing == MMC_TIMING_MMC_DDR52 ||
> +	    (timing == MMC_TIMING_MMC_HS400 &&
> +	    clock == MMC_HS200_MAX_DTR) ||
>  	    host->flags & SDHCI_HS400_TUNING)
>  		return 2;
>  	return 1;
>  }
>  
>  static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
> -					    unsigned int clock)
> +					    unsigned int clock,
> +					    unsigned int timing)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> -	struct mmc_ios curr_ios = host->mmc->ios;
>  	struct clk *core_clk = msm_host->bulk_clks[0].clk;
>  	unsigned long achieved_rate;
>  	unsigned int desired_rate;
>  	unsigned int mult;
>  	int rc;
>  
> -	mult = msm_get_clock_mult_for_bus_mode(host);
> +	mult = msm_get_clock_mult_for_bus_mode(host, clock, timing);
>  	desired_rate = clock * mult;
>  	rc = dev_pm_opp_set_rate(mmc_dev(host->mmc), desired_rate);
>  	if (rc) {
>  		pr_err("%s: Failed to set clock at rate %u at timing %d\n",
> -		       mmc_hostname(host->mmc), desired_rate, curr_ios.timing);
> +		       mmc_hostname(host->mmc), desired_rate, timing);
>  		return;
>  	}
>  
> @@ -397,7 +399,7 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>  	msm_host->clk_rate = desired_rate;
>  
>  	pr_debug("%s: Setting clock at rate %lu at timing %d\n",
> -		 mmc_hostname(host->mmc), achieved_rate, curr_ios.timing);
> +		 mmc_hostname(host->mmc), achieved_rate, timing);
>  }
>  
>  /* Platform specific tuning */
> @@ -1239,7 +1241,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  	 */
>  	if (host->flags & SDHCI_HS400_TUNING) {
>  		sdhci_msm_hc_select_mode(host);
> -		msm_set_clock_rate_for_bus_mode(host, ios.clock);
> +		msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
>  		host->flags &= ~SDHCI_HS400_TUNING;
>  	}
>  
> @@ -1864,6 +1866,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct mmc_ios ios = host->mmc->ios;
>  
>  	if (!clock) {
>  		host->mmc->actual_clock = msm_host->clk_rate = 0;
> @@ -1872,7 +1875,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	sdhci_msm_hc_select_mode(host);
>  
> -	msm_set_clock_rate_for_bus_mode(host, clock);
> +	msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
>  out:
>  	__sdhci_msm_set_clock(host, clock);
>  }
> -- 
> 2.34.1
> 
>