From 56fc950260216d110907f41c916947d757513a83 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Mon, 14 Dec 2020 09:21:14 -0800 Subject: [PATCH 1/3] mmc: sdhci-msm: Warn about overclocking SD/MMC As talked about in commit 5e4b7e82d497 ("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks"), most clocks handled by the Qualcomm Technologies, Inc. clock drivers are rounded _up_ by default instead of down. We should make sure SD/MMC clocks are always rounded down in the clock drivers. Let's add a warning in the Qualcomm Technologies, Inc. SDHCI driver to help catch the problem. This would have saved a bunch of time [1]. NOTE: this doesn't actually fix any problem, it just makes it obvious to devs that there is a problem and that should be an indication to fix the clock driver. Change-Id: I0d7b15c854eb716383bee54715361d8936b6ad0f Git-Repo: https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git/ Git-Commit: a8cd989e1a57dff3994cd113650afb0223c44ec6 Suggested-by: Stephen Boyd Signed-off-by: Douglas Anderson Reviewed-by: Stephen Boyd Reviewed-by: Bjorn Andersson Acked-by: Adrian Hunter Signed-off-by: Sarthak Garg --- drivers/mmc/host/sdhci-msm.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 8718cd19970c..343ed57bba6c 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -560,6 +560,7 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *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; int rc; clock = msm_get_clock_rate_for_bus_mode(host, clock); @@ -570,10 +571,20 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host, curr_ios.timing); return; } + + /* + * Qualcomm Technologies, Inc. clock drivers by default round + * clock _up_ if they can't make the requested rate. This is not + * good for SD. Yell if we encounter it. + */ + achieved_rate = clk_get_rate(core_clk); + if (achieved_rate > clock) + pr_debug("%s: Card appears overclocked; req %u Hz, actual %lu Hz\n", + mmc_hostname(host->mmc), clock, achieved_rate); + msm_host->clk_rate = clock; pr_debug("%s: Setting clock at rate %lu at timing %d\n", - mmc_hostname(host->mmc), clk_get_rate(core_clk), - curr_ios.timing); + mmc_hostname(host->mmc), achieved_rate, curr_ios.timing); } /* Platform specific tuning */ From 76d7b8f192801d982ccf7012df7440fcfc4598e0 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Mon, 14 Dec 2020 09:21:15 -0800 Subject: [PATCH 2/3] mmc: sdhci-msm: Actually set the actual clock The MSM SDHCI driver always set the "actual_clock" field to 0. It had a comment about it not being needed because we weren't using the standard SDHCI divider mechanism and we'd just fallback to "host->clock". However, it's still better to provide the actual clock. Why? 1. It will make timeout calculations slightly better. On one system I have, the eMMC requets 200 MHz (for HS400-ES) but actually gets 192 MHz. These are close, but why not get the more accurate one. 2. If things are seriously off in the clock driver and it's missing rates or picking the wrong rate (maybe it's rounding up instead of down), this will make it much more obvious what's going on. NOTE: we have to be a little careful here because the "actual_clock" field shouldn't include the multiplier that sdhci-msm needs internally. Change-Id: I16f0ad021209e7a0c2797c695e972888b542246f Git-Repo: https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git/ Git-Commit: f16c8fd4449efb4441272af6102e55523b15a7ad Suggested-by: Adrian Hunter Signed-off-by: Douglas Anderson Reviewed-by: Bjorn Andersson Acked-by: Adrian Hunter Reviewed-by: Veerabhadrarao Badiganti Signed-off-by: Sarthak Garg --- drivers/mmc/host/sdhci-msm.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 343ed57bba6c..37d23f8446dd 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -520,8 +520,7 @@ static void sdhci_msm_v5_variant_writel_relaxed(u32 val, writel_relaxed(val, host->ioaddr + offset); } -static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host, - unsigned int clock) +static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host) { struct mmc_ios ios = host->mmc->ios; /* @@ -534,8 +533,8 @@ static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host, ios.timing == MMC_TIMING_MMC_DDR52 || ios.timing == MMC_TIMING_MMC_HS400 || host->flags & SDHCI_HS400_TUNING) - clock *= 2; - return clock; + return 2; + return 1; } #if defined(CONFIG_SDC_QTI) @@ -561,14 +560,16 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *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; - clock = msm_get_clock_rate_for_bus_mode(host, clock); - rc = clk_set_rate(core_clk, clock); + mult = msm_get_clock_mult_for_bus_mode(host); + desired_rate = clock * mult; + rc = clk_set_rate(core_clk, desired_rate); if (rc) { pr_err("%s: Failed to set clock at rate %u at timing %d\n", - mmc_hostname(host->mmc), clock, - curr_ios.timing); + mmc_hostname(host->mmc), desired_rate, curr_ios.timing); return; } @@ -578,11 +579,14 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host, * good for SD. Yell if we encounter it. */ achieved_rate = clk_get_rate(core_clk); - if (achieved_rate > clock) + if (achieved_rate > desired_rate) pr_debug("%s: Card appears overclocked; req %u Hz, actual %lu Hz\n", - mmc_hostname(host->mmc), clock, achieved_rate); + mmc_hostname(host->mmc), desired_rate, achieved_rate); + host->mmc->actual_clock = achieved_rate / mult; + + /* Stash the rate we requested to use in sdhci_msm_runtime_resume() */ + msm_host->clk_rate = desired_rate; - msm_host->clk_rate = clock; pr_debug("%s: Setting clock at rate %lu at timing %d\n", mmc_hostname(host->mmc), achieved_rate, curr_ios.timing); } @@ -2480,13 +2484,6 @@ static unsigned int sdhci_msm_get_sup_clk_rate(struct sdhci_host *host, static void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) { u16 clk; - /* - * Keep actual_clock as zero - - * - since there is no divider used so no need of having actual_clock. - * - MSM controller uses SDCLK for data timeout calculation. If - * actual_clock is zero, host->clock is taken for calculation. - */ - host->mmc->actual_clock = 0; sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); @@ -2509,7 +2506,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); if (!clock) { - msm_host->clk_rate = clock; + host->mmc->actual_clock = msm_host->clk_rate = 0; goto out; } From eb133f0db5e1768ea76cb5c39fa081d5ff21a972 Mon Sep 17 00:00:00 2001 From: Sarthak Garg Date: Tue, 23 Feb 2021 18:07:55 +0530 Subject: [PATCH 3/3] mmc: sdhci-msm: Use actual clock for mclk_freq calculation Due to SSC (Spread Spectrum) sdcc2 clock wont be 202Mhz but will be slightly less than 202Mhz (201.99). For yupik we are using floor ops in clock's driver to take the floor value for the frequency passed from sdhc driver. For 201.99Mhz clk_round_rate API will also round to the floor value leading to 100Mhz for our mclk_freq calculation. Instead of clk_round_rate and dividing by two logic we can directly use actual clock which will be supplied to the card in mclk_freq calculation. Change-Id: I57a864b98d85573eb9038b6d6f502f83850eed73 Signed-off-by: Sarthak Garg --- drivers/mmc/host/sdhci-msm.c | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 37d23f8446dd..aa1a815a380b 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -475,9 +475,6 @@ static void sdhci_msm_bus_voting(struct sdhci_host *host, bool enable); static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name, u32 **bw_vecs, int *len, u32 size); -static unsigned int sdhci_msm_get_sup_clk_rate(struct sdhci_host *host, - u32 req_clk); - static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -835,7 +832,7 @@ static int msm_init_cm_dll(struct sdhci_host *host, const struct sdhci_msm_offset *msm_offset = msm_host->offset; - dll_clock = sdhci_msm_get_sup_clk_rate(host, host->clock); + dll_clock = mmc->actual_clock; spin_lock_irqsave(&host->lock, flags); core_vendor_spec = readl_relaxed(host->ioaddr + @@ -2452,27 +2449,6 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host) return SDHCI_MSM_MIN_CLOCK; } -static unsigned int sdhci_msm_get_sup_clk_rate(struct sdhci_host *host, - u32 req_clk) -{ - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); - struct clk *core_clk = msm_host->bulk_clks[0].clk; - unsigned int sup_clk = -1; - - if (req_clk < sdhci_msm_get_min_clock(host)) { - sup_clk = sdhci_msm_get_min_clock(host); - return sup_clk; - } - - sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk)); - - if (host->clock != msm_host->clk_rate) - sup_clk = sup_clk / 2; - - return sup_clk; -} - /** * __sdhci_msm_set_clock - sdhci_msm clock control. *