[PATCH v5 1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC

[PATCH v5 1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC

[Douglas Anderson]

As talked about in commit 5e4b7e82d497 ("clk: qcom: gcc-sdm845: Use
floor ops for sdcc clks"), most clocks handled by the Qualcomm 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 SDHCI driver to help catch the
problem.

This would have saved a bunch of time [1].

NOTE: this doesn't actually fix any problems, it just makes it obvious
to devs that there is a problem and that should be an indication to
fix the clock driver.

[1] http://lore.kernel.org/r/20201210102234.1.I096779f219625148900fc984dd0084ed1ba87c7f@changeid

Suggested-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

(no changes since v4)

Changes in v4:
- Emphasize in the commit message that this itself doesn't fix anything.

Changes in v3:
- Proper printf format code.

Changes in v2:
- Store rate in unsigned long, not unsigned int.
- Reuse the clk_get_rate() in the later print.

 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 3451eb325513..50beb407dbe9 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -353,6 +353,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);
@@ -363,10 +364,20 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
 		       curr_ios.timing);
 		return;
 	}
+
+	/*
+	 * Qualcomm 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_warn("%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 */
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v5 2/2] mmc: sdhci-msm: Actually set the actual clock

[Douglas Anderson]

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.

Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes in v5:
- Remove unused clock parameter.
- Add a comment that we're stashing the requested rate.

Changes in v4:
- ("mmc: sdhci-msm: Actually set the actual clock") new for v4.

 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 50beb407dbe9..f5669dc858d0 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -328,8 +328,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;
 	/*
@@ -342,8 +341,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;
 }
 
 static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
@@ -354,14 +353,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 = dev_pm_opp_set_rate(mmc_dev(host->mmc), clock);
+	mult = msm_get_clock_mult_for_bus_mode(host);
+	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), clock,
-		       curr_ios.timing);
+		       mmc_hostname(host->mmc), desired_rate, curr_ios.timing);
 		return;
 	}
 
@@ -371,11 +372,14 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
 	 * encounter it.
 	 */
 	achieved_rate = clk_get_rate(core_clk);
-	if (achieved_rate > clock)
+	if (achieved_rate > desired_rate)
 		pr_warn("%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);
 }
@@ -1756,13 +1760,6 @@ static unsigned int sdhci_msm_get_min_clock(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);
 
@@ -1785,7 +1782,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;
 	}
 
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v5 1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC

[Adrian Hunter]

On 14/12/20 7:21 pm, Douglas Anderson wrote:
> As talked about in commit 5e4b7e82d497 ("clk: qcom: gcc-sdm845: Use
> floor ops for sdcc clks"), most clocks handled by the Qualcomm 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 SDHCI driver to help catch the
> problem.
> 
> This would have saved a bunch of time [1].
> 
> NOTE: this doesn't actually fix any problems, it just makes it obvious
> to devs that there is a problem and that should be an indication to
> fix the clock driver.
> 
> [1] http://lore.kernel.org/r/20201210102234.1.I096779f219625148900fc984dd0084ed1ba87c7f@changeid
> 
> Suggested-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 
> (no changes since v4)
> 
> Changes in v4:
> - Emphasize in the commit message that this itself doesn't fix anything.
> 
> Changes in v3:
> - Proper printf format code.
> 
> Changes in v2:
> - Store rate in unsigned long, not unsigned int.
> - Reuse the clk_get_rate() in the later print.
> 
>  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 3451eb325513..50beb407dbe9 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -353,6 +353,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);
> @@ -363,10 +364,20 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>  		       curr_ios.timing);
>  		return;
>  	}
> +
> +	/*
> +	 * Qualcomm 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_warn("%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 */
> 

Re: [PATCH v5 2/2] mmc: sdhci-msm: Actually set the actual clock

[Adrian Hunter]

On 14/12/20 7:21 pm, Douglas Anderson wrote:
> 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.
> 
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 
> Changes in v5:
> - Remove unused clock parameter.
> - Add a comment that we're stashing the requested rate.
> 
> Changes in v4:
> - ("mmc: sdhci-msm: Actually set the actual clock") new for v4.
> 
>  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 50beb407dbe9..f5669dc858d0 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -328,8 +328,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;
>  	/*
> @@ -342,8 +341,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;
>  }
>  
>  static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
> @@ -354,14 +353,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 = dev_pm_opp_set_rate(mmc_dev(host->mmc), clock);
> +	mult = msm_get_clock_mult_for_bus_mode(host);
> +	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), clock,
> -		       curr_ios.timing);
> +		       mmc_hostname(host->mmc), desired_rate, curr_ios.timing);
>  		return;
>  	}
>  
> @@ -371,11 +372,14 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>  	 * encounter it.
>  	 */
>  	achieved_rate = clk_get_rate(core_clk);
> -	if (achieved_rate > clock)
> +	if (achieved_rate > desired_rate)
>  		pr_warn("%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);
>  }
> @@ -1756,13 +1760,6 @@ static unsigned int sdhci_msm_get_min_clock(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);
>  
> @@ -1785,7 +1782,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;
>  	}
>  
> 

Re: [PATCH v5 2/2] mmc: sdhci-msm: Actually set the actual clock

[Veerabhadrarao Badiganti]

On 12/14/2020 10:51 PM, Douglas Anderson wrote:
> 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.
>
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>

> ---
>
> Changes in v5:
> - Remove unused clock parameter.
> - Add a comment that we're stashing the requested rate.
>
> Changes in v4:
> - ("mmc: sdhci-msm: Actually set the actual clock") new for v4.
>
>   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 50beb407dbe9..f5669dc858d0 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -328,8 +328,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;
>   	/*
> @@ -342,8 +341,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;
>   }
>   
>   static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
> @@ -354,14 +353,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 = dev_pm_opp_set_rate(mmc_dev(host->mmc), clock);
> +	mult = msm_get_clock_mult_for_bus_mode(host);
> +	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), clock,
> -		       curr_ios.timing);
> +		       mmc_hostname(host->mmc), desired_rate, curr_ios.timing);
>   		return;
>   	}
>   
> @@ -371,11 +372,14 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>   	 * encounter it.
>   	 */
>   	achieved_rate = clk_get_rate(core_clk);
> -	if (achieved_rate > clock)
> +	if (achieved_rate > desired_rate)
>   		pr_warn("%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);
>   }
> @@ -1756,13 +1760,6 @@ static unsigned int sdhci_msm_get_min_clock(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);
>   
> @@ -1785,7 +1782,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;
>   	}
>   

Re: [PATCH v5 1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC

[Doug Anderson]

Ulf,

On Mon, Dec 14, 2020 at 9:23 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> As talked about in commit 5e4b7e82d497 ("clk: qcom: gcc-sdm845: Use
> floor ops for sdcc clks"), most clocks handled by the Qualcomm 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 SDHCI driver to help catch the
> problem.
>
> This would have saved a bunch of time [1].
>
> NOTE: this doesn't actually fix any problems, it just makes it obvious
> to devs that there is a problem and that should be an indication to
> fix the clock driver.
>
> [1] http://lore.kernel.org/r/20201210102234.1.I096779f219625148900fc984dd0084ed1ba87c7f@changeid
>
> Suggested-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> (no changes since v4)
>
> Changes in v4:
> - Emphasize in the commit message that this itself doesn't fix anything.
>
> Changes in v3:
> - Proper printf format code.
>
> Changes in v2:
> - Store rate in unsigned long, not unsigned int.
> - Reuse the clk_get_rate() in the later print.
>
>  drivers/mmc/host/sdhci-msm.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Is there anything you need me to do for this patch and the next one?
They are both reviewed / Acked and hopefully have sat around long
enough that folks who took a long holiday break had a chance to shout
if they were going to, so I think they could land.  ;-)  Please yell
if there's something you need me to do, or feel free to tell me to sit
quietly and be patient.

Thanks!

-Doug

Re: [PATCH v5 1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC

[Ulf Hansson]

On Mon, 14 Dec 2020 at 18:23, Douglas Anderson <dianders@chromium.org> wrote:
>
> As talked about in commit 5e4b7e82d497 ("clk: qcom: gcc-sdm845: Use
> floor ops for sdcc clks"), most clocks handled by the Qualcomm 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 SDHCI driver to help catch the
> problem.
>
> This would have saved a bunch of time [1].
>
> NOTE: this doesn't actually fix any problems, it just makes it obvious
> to devs that there is a problem and that should be an indication to
> fix the clock driver.
>
> [1] http://lore.kernel.org/r/20201210102234.1.I096779f219625148900fc984dd0084ed1ba87c7f@changeid
>
> Suggested-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Applied for next, thanks!

Kind regards
Uffe


> ---
>
> (no changes since v4)
>
> Changes in v4:
> - Emphasize in the commit message that this itself doesn't fix anything.
>
> Changes in v3:
> - Proper printf format code.
>
> Changes in v2:
> - Store rate in unsigned long, not unsigned int.
> - Reuse the clk_get_rate() in the later print.
>
>  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 3451eb325513..50beb407dbe9 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -353,6 +353,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);
> @@ -363,10 +364,20 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>                        curr_ios.timing);
>                 return;
>         }
> +
> +       /*
> +        * Qualcomm 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_warn("%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 */
> --
> 2.29.2.576.ga3fc446d84-goog
>

Re: [PATCH v5 2/2] mmc: sdhci-msm: Actually set the actual clock

[Ulf Hansson]

On Mon, 14 Dec 2020 at 18:23, Douglas Anderson <dianders@chromium.org> wrote:
>
> 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.
>
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Applied for next, thanks!

Kind regards
Uffe


> ---
>
> Changes in v5:
> - Remove unused clock parameter.
> - Add a comment that we're stashing the requested rate.
>
> Changes in v4:
> - ("mmc: sdhci-msm: Actually set the actual clock") new for v4.
>
>  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 50beb407dbe9..f5669dc858d0 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -328,8 +328,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;
>         /*
> @@ -342,8 +341,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;
>  }
>
>  static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
> @@ -354,14 +353,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 = dev_pm_opp_set_rate(mmc_dev(host->mmc), clock);
> +       mult = msm_get_clock_mult_for_bus_mode(host);
> +       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), clock,
> -                      curr_ios.timing);
> +                      mmc_hostname(host->mmc), desired_rate, curr_ios.timing);
>                 return;
>         }
>
> @@ -371,11 +372,14 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>          * encounter it.
>          */
>         achieved_rate = clk_get_rate(core_clk);
> -       if (achieved_rate > clock)
> +       if (achieved_rate > desired_rate)
>                 pr_warn("%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);
>  }
> @@ -1756,13 +1760,6 @@ static unsigned int sdhci_msm_get_min_clock(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);
>
> @@ -1785,7 +1782,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;
>         }
>
> --
> 2.29.2.576.ga3fc446d84-goog
>

Re: [PATCH v5 1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC

[patchwork-bot+linux-arm-msm]

Hello:

This series was applied to qcom/linux.git (refs/heads/for-next):

On Mon, 14 Dec 2020 09:21:14 -0800 you wrote:
> As talked about in commit 5e4b7e82d497 ("clk: qcom: gcc-sdm845: Use
> floor ops for sdcc clks"), most clocks handled by the Qualcomm 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 SDHCI driver to help catch the
> problem.
> 
> [...]

Here is the summary with links:
  - [v5,1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC
    https://git.kernel.org/qcom/c/a8cd989e1a57
  - [v5,2/2] mmc: sdhci-msm: Actually set the actual clock
    https://git.kernel.org/qcom/c/f16c8fd4449e

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html