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

Message ID 20201214092048.v5.1.Iec3430c7d3c2a29262695edef7b82a14aaa567e5@changeid
State New
Headers show
Series
  • [v5,1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC
Related show

Commit Message

Doug Anderson Dec. 14, 2020, 5:21 p.m.
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(-)

Comments

Adrian Hunter Dec. 15, 2020, 8:24 a.m. | #1
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 */

>
Adrian Hunter Dec. 15, 2020, 8:24 a.m. | #2
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;
>  	}
>  
>
Veerabhadrarao Badiganti Dec. 15, 2020, 8:44 a.m. | #3
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;

>   	}

>
Doug Anderson Jan. 8, 2021, 9:21 p.m. | #4
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
Ulf Hansson Jan. 11, 2021, 6:06 p.m. | #5
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

>

Patch

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 */