| Message ID | 20201211091150.v4.2.I7564620993acd4baa63fa0e3925ca879a86d3ee3@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] mmc: sdhci-msm: Warn about overclocking SD/MMC | expand |
On 12/11/2020 10:42 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. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v4: > - ("mmc: sdhci-msm: Actually set the actual clock") new for v4. > > drivers/mmc/host/sdhci-msm.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 50beb407dbe9..08a3960001ad 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -328,7 +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, > +static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host, > unsigned int clock) nit: clock variable not being used anymore. We can drop it. > { > struct mmc_ios ios = host->mmc->ios; > @@ -342,8 +342,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 +354,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, clock); > + 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 +373,12 @@ 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; > > - msm_host->clk_rate = clock; > + msm_host->clk_rate = desired_rate; Can you set msm_host->clk_rate also to achieved_rate? At few places in this driver, host->clock is being used where achieved_rate should be used ideally. I will replace those instances with 'msm_host->clk_rate' in a separate patch once this change merged. > pr_debug("%s: Setting clock at rate %lu at timing %d\n", > mmc_hostname(host->mmc), achieved_rate, curr_ios.timing); > } > @@ -1756,13 +1759,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 +1781,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; > } >
On Fri 11 Dec 11:12 CST 2020, 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. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > > Changes in v4: > - ("mmc: sdhci-msm: Actually set the actual clock") new for v4. > > drivers/mmc/host/sdhci-msm.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 50beb407dbe9..08a3960001ad 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -328,7 +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, > +static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host, > unsigned int clock) > { > struct mmc_ios ios = host->mmc->ios; > @@ -342,8 +342,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 +354,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, clock); > + 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 +373,12 @@ 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; > > - msm_host->clk_rate = clock; > + 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); > } > @@ -1756,13 +1759,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 +1781,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 >
Hi, On Mon, Dec 14, 2020 at 4:44 AM Veerabhadrarao Badiganti <vbadigan@codeaurora.org> wrote: > > > On 12/11/2020 10:42 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. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > Changes in v4: > > - ("mmc: sdhci-msm: Actually set the actual clock") new for v4. > > > > drivers/mmc/host/sdhci-msm.c | 32 ++++++++++++++------------------ > > 1 file changed, 14 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > > index 50beb407dbe9..08a3960001ad 100644 > > --- a/drivers/mmc/host/sdhci-msm.c > > +++ b/drivers/mmc/host/sdhci-msm.c > > @@ -328,7 +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, > > +static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host, > > unsigned int clock) > > nit: clock variable not being used anymore. We can drop it. Good point. Sending out a v5 with this. > > { > > struct mmc_ios ios = host->mmc->ios; > > @@ -342,8 +342,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 +354,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, clock); > > + 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 +373,12 @@ 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; > > > > - msm_host->clk_rate = clock; > > + msm_host->clk_rate = desired_rate; > > > Can you set msm_host->clk_rate also to achieved_rate? Personally I'd rather not, but if you are sure that's what you want I won't object to it too strongly. Why do I feel this way? The member "clk_rate" contains the value that we passed to dev_pm_opp_set_rate() the first time and I'd rather use that exact same value in sdhci_msm_runtime_resume(). Mostly I'm just being paranoid in case there is a bug and the operations aren't "stable". For instance, let's imagine a fictional case where somewhere in the clock framework there is a transition to kHz (something like this _actually_ happens in the DRM subsystem): clk_set_rate(rate_hz): rate_khz = rate_hz / 1000; real_clk_set_rate(rate_khz); real_clk_set_rate(rate_khz) rate_hz = rate_khz * 1000; for each table_rate in table: if table_rate <= rate_hz: break; set_hw_rate(table_rate); real_clk_get_rate() rate_hz = get_hw_rate(); return rate_hz / 1000; clk_get_rate() rate_khz = real_clk_get_rate() return rate_khz * 1000; Now if your table has these rates: { 111111111, 222222222, 333333333 } Calling clk_set_rate(400000000) will set your rate to 333333333 Hz. Now calling clk_get_rate() will return you 333333000. Now calling clk_set_rate(333333000) will set your rate to 222222222 Hz! IMO the above would be a bug, but I have seen things like that happen. It's safer to stash the actual rate that we _requested_ and, if we need to request the rate again, we pass that same value. That should always work. I added a comment to at least make it look more explicit that we're stashing the requested value. > At few places in this driver, host->clock is being used where > achieved_rate should be used ideally. > I will replace those instances with 'msm_host->clk_rate' in a separate > patch once this change merged. Sounds good, thanks! -Doug
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 50beb407dbe9..08a3960001ad 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -328,7 +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, +static unsigned int msm_get_clock_mult_for_bus_mode(struct sdhci_host *host, unsigned int clock) { struct mmc_ios ios = host->mmc->ios; @@ -342,8 +342,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 +354,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, clock); + 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 +373,12 @@ 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; - msm_host->clk_rate = clock; + 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); } @@ -1756,13 +1759,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 +1781,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; }
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. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v4: - ("mmc: sdhci-msm: Actually set the actual clock") new for v4. drivers/mmc/host/sdhci-msm.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)