* Re: [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition
2025-11-14 8:28 [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition Sarthak Garg
@ 2025-11-14 14:58 ` Bjorn Andersson
2025-11-17 8:55 ` Adrian Hunter
2025-11-17 10:55 ` Ulf Hansson
2 siblings, 0 replies; 5+ messages in thread
From: Bjorn Andersson @ 2025-11-14 14:58 UTC (permalink / raw)
To: Sarthak Garg
Cc: Adrian Hunter, Ulf Hansson, linux-arm-msm, linux-mmc,
linux-kernel, quic_nguyenb, quic_rampraka, quic_pragalla,
quic_sayalil, quic_nitirawa, quic_bhaskarv, kernel
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@oss.qualcomm.com>
Thank you for cleaning that up.
Reviewed-by: Bjorn Andersson <andersson@kernel.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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition
2025-11-14 8:28 [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition Sarthak Garg
2025-11-14 14:58 ` Bjorn Andersson
@ 2025-11-17 8:55 ` Adrian Hunter
2025-11-17 10:55 ` Ulf Hansson
2 siblings, 0 replies; 5+ messages in thread
From: Adrian Hunter @ 2025-11-17 8:55 UTC (permalink / raw)
To: Sarthak Garg, Ulf Hansson
Cc: linux-arm-msm, linux-mmc, linux-kernel, quic_nguyenb,
quic_rampraka, quic_pragalla, quic_sayalil, quic_nitirawa,
quic_bhaskarv, kernel
On 14/11/2025 10:28, 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>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> 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);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition
2025-11-14 8:28 [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition Sarthak Garg
2025-11-14 14:58 ` Bjorn Andersson
2025-11-17 8:55 ` Adrian Hunter
@ 2025-11-17 10:55 ` Ulf Hansson
2025-11-25 13:00 ` Ulf Hansson
2 siblings, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2025-11-17 10:55 UTC (permalink / raw)
To: Sarthak Garg
Cc: Adrian Hunter, linux-arm-msm, linux-mmc, linux-kernel,
quic_nguyenb, quic_rampraka, quic_pragalla, quic_sayalil,
quic_nitirawa, quic_bhaskarv, kernel
On Fri, 14 Nov 2025 at 09:28, Sarthak Garg
<sarthak.garg@oss.qualcomm.com> 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>
It sounds to me that we should add a fixes/stable tag for this, right?
Kind regards
Uffe
> ---
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] mmc: sdhci-msm: Avoid early clock doubling during HS400 transition
2025-11-17 10:55 ` Ulf Hansson
@ 2025-11-25 13:00 ` Ulf Hansson
0 siblings, 0 replies; 5+ messages in thread
From: Ulf Hansson @ 2025-11-25 13:00 UTC (permalink / raw)
To: Sarthak Garg
Cc: Adrian Hunter, linux-arm-msm, linux-mmc, linux-kernel,
quic_nguyenb, quic_rampraka, quic_pragalla, quic_sayalil,
quic_nitirawa, quic_bhaskarv, kernel
On Mon, 17 Nov 2025 at 11:55, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 14 Nov 2025 at 09:28, Sarthak Garg
> <sarthak.garg@oss.qualcomm.com> 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>
>
> It sounds to me that we should add a fixes/stable tag for this, right?
I applied this for next and added a stable tag, please let me know if
you prefer something different.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 5+ messages in thread