[PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[Douglas Anderson]

I would repeat the same commit message that was in commit 5e4b7e82d497
("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks") but it seems
silly to do so when you could just go read that commit.

NOTE: this is actually extra terrible because we're missing the 50 MHz
rate in the table (see the next patch AKA ("clk: qcom: gcc-sc7180: Add
50 MHz clock rate for SDC2")).  That means then when you run an older
SD card it'll try to clock it at 100 MHz when it's only specced to run
at 50 MHz max.  As you can probably guess that doesn't work super
well.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
---
Taniya: can you please update whatever process is used to generate
these clock files to use floor for SD card clocks.  I hope you can
also scour through these files looking for similar problems on other
SoCs and submit patches for them.

 drivers/clk/qcom/gcc-sc7180.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index 68d8f7aaf64e..b080739ab0c3 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -642,7 +642,7 @@ static struct clk_rcg2 gcc_sdcc1_ice_core_clk_src = {
 		.name = "gcc_sdcc1_ice_core_clk_src",
 		.parent_data = gcc_parent_data_0,
 		.num_parents = 4,
-		.ops = &clk_rcg2_ops,
+		.ops = &clk_rcg2_floor_ops,
 	},
 };
 
@@ -666,7 +666,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
 		.name = "gcc_sdcc2_apps_clk_src",
 		.parent_data = gcc_parent_data_5,
 		.num_parents = 5,
-		.ops = &clk_rcg2_ops,
+		.ops = &clk_rcg2_floor_ops,
 	},
 };
 
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH 2/2] clk: qcom: gcc-sc7180: Add 50 MHz clock rate for SDC2

[Douglas Anderson]

50 MHz is an incredibly common clock rate for SD cards to run at.
It's "high speed" mode in SD (not very fast these days, but it used to
be) or:
  #define HIGH_SPEED_MAX_DTR	50000000

If we don't support this then older "high speed" cards can only run at
25 MHz or at half their normal speed.  There doesn't seem to be any
reason to skip this clock rate, so add it.

Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/clk/qcom/gcc-sc7180.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index b080739ab0c3..d82d725ac231 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -651,6 +651,7 @@ static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
 	F(9600000, P_BI_TCXO, 2, 0, 0),
 	F(19200000, P_BI_TCXO, 1, 0, 0),
 	F(25000000, P_GPLL0_OUT_EVEN, 12, 0, 0),
+	F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0),
 	F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0),
 	F(202000000, P_GPLL7_OUT_MAIN, 4, 0, 0),
 	{ }
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH 2/2] clk: qcom: gcc-sc7180: Add 50 MHz clock rate for SDC2

[Bjorn Andersson]

On Thu 10 Dec 12:22 CST 2020, Douglas Anderson wrote:

> 50 MHz is an incredibly common clock rate for SD cards to run at.
> It's "high speed" mode in SD (not very fast these days, but it used to
> be) or:
>   #define HIGH_SPEED_MAX_DTR	50000000
> 
> If we don't support this then older "high speed" cards can only run at
> 25 MHz or at half their normal speed.  There doesn't seem to be any
> reason to skip this clock rate, so add it.
> 
> Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 
>  drivers/clk/qcom/gcc-sc7180.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> index b080739ab0c3..d82d725ac231 100644
> --- a/drivers/clk/qcom/gcc-sc7180.c
> +++ b/drivers/clk/qcom/gcc-sc7180.c
> @@ -651,6 +651,7 @@ static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
>  	F(9600000, P_BI_TCXO, 2, 0, 0),
>  	F(19200000, P_BI_TCXO, 1, 0, 0),
>  	F(25000000, P_GPLL0_OUT_EVEN, 12, 0, 0),
> +	F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0),
>  	F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0),
>  	F(202000000, P_GPLL7_OUT_MAIN, 4, 0, 0),
>  	{ }
> -- 
> 2.29.2.576.ga3fc446d84-goog
> 

Re: [PATCH 2/2] clk: qcom: gcc-sc7180: Add 50 MHz clock rate for SDC2

[Stephen Boyd]

Quoting Douglas Anderson (2020-12-10 10:22:39)
> 50 MHz is an incredibly common clock rate for SD cards to run at.
> It's "high speed" mode in SD (not very fast these days, but it used to
> be) or:
>   #define HIGH_SPEED_MAX_DTR    50000000
> 
> If we don't support this then older "high speed" cards can only run at
> 25 MHz or at half their normal speed.  There doesn't seem to be any
> reason to skip this clock rate, so add it.
> 
> Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Applied to clk-next

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[Bjorn Andersson]

On Thu 10 Dec 12:22 CST 2020, Douglas Anderson wrote:

> I would repeat the same commit message that was in commit 5e4b7e82d497
> ("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks") but it seems
> silly to do so when you could just go read that commit.
> 
> NOTE: this is actually extra terrible because we're missing the 50 MHz
> rate in the table (see the next patch AKA ("clk: qcom: gcc-sc7180: Add
> 50 MHz clock rate for SDC2")).  That means then when you run an older
> SD card it'll try to clock it at 100 MHz when it's only specced to run
> at 50 MHz max.  As you can probably guess that doesn't work super
> well.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
> ---
> Taniya: can you please update whatever process is used to generate
> these clock files to use floor for SD card clocks.  I hope you can
> also scour through these files looking for similar problems on other
> SoCs and submit patches for them.
> 
>  drivers/clk/qcom/gcc-sc7180.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> index 68d8f7aaf64e..b080739ab0c3 100644
> --- a/drivers/clk/qcom/gcc-sc7180.c
> +++ b/drivers/clk/qcom/gcc-sc7180.c
> @@ -642,7 +642,7 @@ static struct clk_rcg2 gcc_sdcc1_ice_core_clk_src = {
>  		.name = "gcc_sdcc1_ice_core_clk_src",
>  		.parent_data = gcc_parent_data_0,
>  		.num_parents = 4,
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_floor_ops,
>  	},
>  };
>  
> @@ -666,7 +666,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
>  		.name = "gcc_sdcc2_apps_clk_src",
>  		.parent_data = gcc_parent_data_5,
>  		.num_parents = 5,
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_floor_ops,
>  	},
>  };
>  
> -- 
> 2.29.2.576.ga3fc446d84-goog
> 

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[Stephen Boyd]

Quoting Douglas Anderson (2020-12-10 10:22:38)
> I would repeat the same commit message that was in commit 5e4b7e82d497
> ("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks") but it seems
> silly to do so when you could just go read that commit.
> 
> NOTE: this is actually extra terrible because we're missing the 50 MHz
> rate in the table (see the next patch AKA ("clk: qcom: gcc-sc7180: Add
> 50 MHz clock rate for SDC2")).  That means then when you run an older
> SD card it'll try to clock it at 100 MHz when it's only specced to run
> at 50 MHz max.  As you can probably guess that doesn't work super
> well.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
> ---
> Taniya: can you please update whatever process is used to generate
> these clock files to use floor for SD card clocks.  I hope you can
> also scour through these files looking for similar problems on other
> SoCs and submit patches for them.

Any chance the consumer of this clk can call clk_round_rate() and WARN()
if the rate is not anywhere near what it wants it to be? I fear that
this problem will just keep coming up otherwise.

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 12:27 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Douglas Anderson (2020-12-10 10:22:38)
> > I would repeat the same commit message that was in commit 5e4b7e82d497
> > ("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks") but it seems
> > silly to do so when you could just go read that commit.
> >
> > NOTE: this is actually extra terrible because we're missing the 50 MHz
> > rate in the table (see the next patch AKA ("clk: qcom: gcc-sc7180: Add
> > 50 MHz clock rate for SDC2")).  That means then when you run an older
> > SD card it'll try to clock it at 100 MHz when it's only specced to run
> > at 50 MHz max.  As you can probably guess that doesn't work super
> > well.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
> > ---
> > Taniya: can you please update whatever process is used to generate
> > these clock files to use floor for SD card clocks.  I hope you can
> > also scour through these files looking for similar problems on other
> > SoCs and submit patches for them.
>
> Any chance the consumer of this clk can call clk_round_rate() and WARN()
> if the rate is not anywhere near what it wants it to be? I fear that
> this problem will just keep coming up otherwise.

Good idea.  Posted ("[PATCH] mmc: sdhci-msm: Warn about overclocking
SD/MMC") [1].

I just checked for the clock being higher, not lower and I did a
normal print rather than a WARN splat.

Interestingly this shows that we were also overclocking the eMMC
during probe (though we end up at a proper rate in the end).  I saw
these transitory errors during boot:

[    6.287870] mmc0: Card appears overclocked; req 52000000 Hz, actual
100000000 Hz
[    6.295811] mmc0: Card appears overclocked; req 52000000 Hz, actual
100000000 Hz
[    6.312573] mmc0: Card appears overclocked; req 104000000 Hz,
actual 192000000 Hz

[1] https://lore.kernel.org/r/20201210125709.1.Iec3430c7d3c2a29262695edef7b82a14aaa567e5@changeid

-Doug

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[Stephen Boyd]

Quoting Douglas Anderson (2020-12-10 10:22:38)
> I would repeat the same commit message that was in commit 5e4b7e82d497
> ("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks") but it seems
> silly to do so when you could just go read that commit.
> 
> NOTE: this is actually extra terrible because we're missing the 50 MHz
> rate in the table (see the next patch AKA ("clk: qcom: gcc-sc7180: Add
> 50 MHz clock rate for SDC2")).  That means then when you run an older
> SD card it'll try to clock it at 100 MHz when it's only specced to run
> at 50 MHz max.  As you can probably guess that doesn't work super
> well.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
> ---

Applied to clk-next

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks

[patchwork-bot+linux-arm-msm]

Hello:

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

On Thu, 10 Dec 2020 10:22:38 -0800 you wrote:
> I would repeat the same commit message that was in commit 5e4b7e82d497
> ("clk: qcom: gcc-sdm845: Use floor ops for sdcc clks") but it seems
> silly to do so when you could just go read that commit.
> 
> NOTE: this is actually extra terrible because we're missing the 50 MHz
> rate in the table (see the next patch AKA ("clk: qcom: gcc-sc7180: Add
> 50 MHz clock rate for SDC2")).  That means then when you run an older
> SD card it'll try to clock it at 100 MHz when it's only specced to run
> at 50 MHz max.  As you can probably guess that doesn't work super
> well.
> 
> [...]

Here is the summary with links:
  - [1/2] clk: qcom: gcc-sc7180: Use floor ops for sdcc clks
    https://git.kernel.org/qcom/c/6d37a8d19283
  - [2/2] clk: qcom: gcc-sc7180: Add 50 MHz clock rate for SDC2
    https://git.kernel.org/qcom/c/043577518f02

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