diff mbox series

clk: qcom: gcc-sm8450: set OPS_PARENT_ENABLE on gcc_sdcc2_apps_clk_src

Message ID 20240427-topic-8450sdc2-v1-1-631cbb59e0e5@linaro.org
State New
Headers show
Series clk: qcom: gcc-sm8450: set OPS_PARENT_ENABLE on gcc_sdcc2_apps_clk_src | expand

Commit Message

Konrad Dybcio April 27, 2024, 12:01 p.m. UTC 
Similar to how it works on other SoCs, the top frequency of the SDHCI2
core clock is generated by a separate PLL (peculiar design choice) that
is not guaranteed to be enabled (why does the clock framework not handle
this by default?).

Add the CLK_OPS_PARENT_ENABLE flag to make sure we're not muxing the
RCG input to a dormant source.

Fixes: db0c944ee92b ("clk: qcom: Add clock driver for SM8450")
Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/gcc-sm8450.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: c0b832517f627ead3388c6f0c74e8ac10ad5774b
change-id: 20240427-topic-8450sdc2-3fcfebe1e8ad

Best regards,

Comments

Stephen Boyd April 30, 2024, 12:21 a.m. UTC | #1 
Quoting Konrad Dybcio (2024-04-27 05:01:07)
> Similar to how it works on other SoCs, the top frequency of the SDHCI2
> core clock is generated by a separate PLL (peculiar design choice) that
> is not guaranteed to be enabled (why does the clock framework not handle
> this by default?).
> 
> Add the CLK_OPS_PARENT_ENABLE flag to make sure we're not muxing the
> RCG input to a dormant source.

The RCG2 hardware hasn't required the parent to be enabled for clk
operations besides for the glitch-free source switch. What scenario is
happening here that's requiring this flag? Is the RCG forcibly enabled
perhaps because the bootloader has left the root enable bit set
(CMD_ROOT_EN)? Or are we changing the parent while the clk framework
thinks the clk is off when it is actually on?

TL;DR: This is papering over a bigger bug.
Konrad Dybcio April 30, 2024, 10:46 a.m. UTC | #2 
On 30.04.2024 2:21 AM, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2024-04-27 05:01:07)
>> Similar to how it works on other SoCs, the top frequency of the SDHCI2
>> core clock is generated by a separate PLL (peculiar design choice) that
>> is not guaranteed to be enabled (why does the clock framework not handle
>> this by default?).
>>
>> Add the CLK_OPS_PARENT_ENABLE flag to make sure we're not muxing the
>> RCG input to a dormant source.
> 
> The RCG2 hardware hasn't required the parent to be enabled for clk
> operations besides for the glitch-free source switch. What scenario is
> happening here that's requiring this flag? Is the RCG forcibly enabled
> perhaps because the bootloader has left the root enable bit set
> (CMD_ROOT_EN)? Or are we changing the parent while the clk framework
> thinks the clk is off when it is actually on?
> 
> TL;DR: This is papering over a bigger bug.

Definitely.


Take a look at:

static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
	F(400000, P_BI_TCXO, 12, 1, 4),
	F(25000000, P_GCC_GPLL0_OUT_EVEN, 12, 0, 0),
	F(50000000, P_GCC_GPLL0_OUT_EVEN, 6, 0, 0),
	F(100000000, P_GCC_GPLL0_OUT_EVEN, 3, 0, 0),
	F(202000000, P_GCC_GPLL9_OUT_MAIN, 4, 0, 0),
	{ }
};

XO and GPLL0 are more or less always on, but GPLL9 is described to only
be used for this specific clock for this specific frequency (perhaps it
feeds something else on the soc but that's besides the point).

Then, the parent input is changed during set_rate, but GPLL9 seems to
never be enabled:


@@ -3272,6 +3274,8 @@ static int gcc_sm8450_probe(struct platform_device *pdev)
        if (IS_ERR(regmap))
                return PTR_ERR(regmap);
 
+       pr_err("GPLL9 is %s at boot\n", trion_pll_is_enabled(&gcc_gpll9, regmap) ? "enabled" : "disabled");
+
        ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
                                       ARRAY_SIZE(gcc_dfs_clocks));
        if (ret)


(+ cruft to make this callable) results in a:

[    1.637318] GPLL9 is disabled at boot


Konrad
Stephen Boyd April 30, 2024, 9:26 p.m. UTC | #3 
Quoting Konrad Dybcio (2024-04-30 03:46:52)
> On 30.04.2024 2:21 AM, Stephen Boyd wrote:
> > Quoting Konrad Dybcio (2024-04-27 05:01:07)
> >> Similar to how it works on other SoCs, the top frequency of the SDHCI2
> >> core clock is generated by a separate PLL (peculiar design choice) that
> >> is not guaranteed to be enabled (why does the clock framework not handle
> >> this by default?).
> >>
> >> Add the CLK_OPS_PARENT_ENABLE flag to make sure we're not muxing the
> >> RCG input to a dormant source.
> > 
> > The RCG2 hardware hasn't required the parent to be enabled for clk
> > operations besides for the glitch-free source switch. What scenario is
> > happening here that's requiring this flag? Is the RCG forcibly enabled
> > perhaps because the bootloader has left the root enable bit set
> > (CMD_ROOT_EN)? Or are we changing the parent while the clk framework
> > thinks the clk is off when it is actually on?
> > 
> > TL;DR: This is papering over a bigger bug.
> 
> Definitely.
> 
> 
> Take a look at:
> 
> static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
>         F(400000, P_BI_TCXO, 12, 1, 4),
>         F(25000000, P_GCC_GPLL0_OUT_EVEN, 12, 0, 0),
>         F(50000000, P_GCC_GPLL0_OUT_EVEN, 6, 0, 0),
>         F(100000000, P_GCC_GPLL0_OUT_EVEN, 3, 0, 0),
>         F(202000000, P_GCC_GPLL9_OUT_MAIN, 4, 0, 0),
>         { }
> };
> 
> XO and GPLL0 are more or less always on, but GPLL9 is described to only
> be used for this specific clock for this specific frequency (perhaps it
> feeds something else on the soc but that's besides the point).
> 
> Then, the parent input is changed during set_rate, but GPLL9 seems to
> never be enabled:

Is the sdcc2 RCG enabled during the set_rate?
Konrad Dybcio May 7, 2024, 1:51 p.m. UTC | #4 
On 4/30/24 23:26, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2024-04-30 03:46:52)
>> On 30.04.2024 2:21 AM, Stephen Boyd wrote:
>>> Quoting Konrad Dybcio (2024-04-27 05:01:07)
>>>> Similar to how it works on other SoCs, the top frequency of the SDHCI2
>>>> core clock is generated by a separate PLL (peculiar design choice) that
>>>> is not guaranteed to be enabled (why does the clock framework not handle
>>>> this by default?).
>>>>
>>>> Add the CLK_OPS_PARENT_ENABLE flag to make sure we're not muxing the
>>>> RCG input to a dormant source.
>>>
>>> The RCG2 hardware hasn't required the parent to be enabled for clk
>>> operations besides for the glitch-free source switch. What scenario is
>>> happening here that's requiring this flag? Is the RCG forcibly enabled
>>> perhaps because the bootloader has left the root enable bit set
>>> (CMD_ROOT_EN)? Or are we changing the parent while the clk framework
>>> thinks the clk is off when it is actually on?
>>>
>>> TL;DR: This is papering over a bigger bug.
>>
>> Definitely.
>>
>>
>> Take a look at:
>>
>> static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
>>          F(400000, P_BI_TCXO, 12, 1, 4),
>>          F(25000000, P_GCC_GPLL0_OUT_EVEN, 12, 0, 0),
>>          F(50000000, P_GCC_GPLL0_OUT_EVEN, 6, 0, 0),
>>          F(100000000, P_GCC_GPLL0_OUT_EVEN, 3, 0, 0),
>>          F(202000000, P_GCC_GPLL9_OUT_MAIN, 4, 0, 0),
>>          { }
>> };
>>
>> XO and GPLL0 are more or less always on, but GPLL9 is described to only
>> be used for this specific clock for this specific frequency (perhaps it
>> feeds something else on the soc but that's besides the point).
>>
>> Then, the parent input is changed during set_rate, but GPLL9 seems to
>> never be enabled:
> 
> Is the sdcc2 RCG enabled during the set_rate?

without PARENT_OPS_ENABLE:

[    3.326891] sdhci_msm 8804000.mmc: Got CD GPIO
[    3.336839] scsi host0: ufshcd
[    3.337105] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
[    3.346339] ------------[ cut here ]------------
[    3.351093] gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
[    3.351114] WARNING: CPU: 1 PID: 11 at drivers/clk/qcom/clk-rcg2.c:133 update_config+0xc8/0xd8

[...]

[    3.610523] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate


with PARENT_OPS_ENABLE:

[    3.331419] sdhci_msm 8804000.mmc: Got CD GPIO
[    3.336569] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
[    3.344795] scsi host0: ufshcd
[    3.355122] qcrypto 1dfa000.crypto: Adding to iommu group 5
[    3.363567] remoteproc remoteproc0: 2400000.remoteproc is available
[    3.364729] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate

after testing it both ways, I realized it wasn't supposed to make a
difference in this regard, but I suppose I can paste both results anyway..

Konrad
Stephen Boyd May 7, 2024, 8:28 p.m. UTC | #5 
Quoting Konrad Dybcio (2024-05-07 06:51:04)
> 
> without PARENT_OPS_ENABLE:
> 
> [    3.326891] sdhci_msm 8804000.mmc: Got CD GPIO
> [    3.336839] scsi host0: ufshcd
> [    3.337105] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
> [    3.346339] ------------[ cut here ]------------
> [    3.351093] gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
> [    3.351114] WARNING: CPU: 1 PID: 11 at drivers/clk/qcom/clk-rcg2.c:133 update_config+0xc8/0xd8
> 
> [...]
> 
> [    3.610523] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate
> 
> 
> with PARENT_OPS_ENABLE:
> 
> [    3.331419] sdhci_msm 8804000.mmc: Got CD GPIO
> [    3.336569] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
> [    3.344795] scsi host0: ufshcd
> [    3.355122] qcrypto 1dfa000.crypto: Adding to iommu group 5
> [    3.363567] remoteproc remoteproc0: 2400000.remoteproc is available
> [    3.364729] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate
> 
> after testing it both ways, I realized it wasn't supposed to make a
> difference in this regard, but I suppose I can paste both results anyway..
> 

Can you share your patch that prints the message? What bit are you
checking in the hardware to determine if the RCG is enabled? Do you also
print the enable count in software?
Konrad Dybcio May 7, 2024, 9:17 p.m. UTC | #6 
On 5/7/24 22:28, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2024-05-07 06:51:04)
>>
>> without PARENT_OPS_ENABLE:
>>
>> [    3.326891] sdhci_msm 8804000.mmc: Got CD GPIO
>> [    3.336839] scsi host0: ufshcd
>> [    3.337105] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
>> [    3.346339] ------------[ cut here ]------------
>> [    3.351093] gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
>> [    3.351114] WARNING: CPU: 1 PID: 11 at drivers/clk/qcom/clk-rcg2.c:133 update_config+0xc8/0xd8
>>
>> [...]
>>
>> [    3.610523] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate
>>
>>
>> with PARENT_OPS_ENABLE:
>>
>> [    3.331419] sdhci_msm 8804000.mmc: Got CD GPIO
>> [    3.336569] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
>> [    3.344795] scsi host0: ufshcd
>> [    3.355122] qcrypto 1dfa000.crypto: Adding to iommu group 5
>> [    3.363567] remoteproc remoteproc0: 2400000.remoteproc is available
>> [    3.364729] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate
>>
>> after testing it both ways, I realized it wasn't supposed to make a
>> difference in this regard, but I suppose I can paste both results anyway..
>>
> 
> Can you share your patch that prints the message? What bit are you
> checking in the hardware to determine if the RCG is enabled? Do you also
> print the enable count in software?

I already reset-ed the tree state, but I added something like

if (rcg->cmd_rcgr == the one in the declaration)
	pr_err("gcc_sdcc2_apps_clk_src is %s\n", clk_is_enabled(hw) ? "ENABLED" : "DISABLED");

to drivers/clk/qcom/clk-rcg2.c : __clk_rcg2_set_rate()

Konrad
Stephen Boyd May 7, 2024, 9:52 p.m. UTC | #7 
Quoting Konrad Dybcio (2024-05-07 14:17:01)
> 
> 
> On 5/7/24 22:28, Stephen Boyd wrote:
> >>
> > 
> > Can you share your patch that prints the message? What bit are you
> > checking in the hardware to determine if the RCG is enabled? Do you also
> > print the enable count in software?
> 
> I already reset-ed the tree state, but I added something like
> 
> if (rcg->cmd_rcgr == the one in the declaration)
>         pr_err("gcc_sdcc2_apps_clk_src is %s\n", clk_is_enabled(hw) ? "ENABLED" : "DISABLED");
> 
> to drivers/clk/qcom/clk-rcg2.c : __clk_rcg2_set_rate()
> 
> 

Ok. You're reading the software state because there isn't an is_enabled
clk_op for RCGs. Can you also read the CMD register (0x0 offset from
base) and check for CMD_ROOT_EN (bit 1) being set? That's what I mean
when I'm talking about the RCG being enabled in hardware. Similarly,
read CMD_ROOT_OFF (bit 31) to see if some child branch of the RCG is
enabled at this time.
Konrad Dybcio June 6, 2024, 11:56 a.m. UTC | #8 
On 7.05.2024 11:52 PM, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2024-05-07 14:17:01)
>>
>>
>> On 5/7/24 22:28, Stephen Boyd wrote:
>>>>
>>>
>>> Can you share your patch that prints the message? What bit are you
>>> checking in the hardware to determine if the RCG is enabled? Do you also
>>> print the enable count in software?
>>
>> I already reset-ed the tree state, but I added something like
>>
>> if (rcg->cmd_rcgr == the one in the declaration)
>>         pr_err("gcc_sdcc2_apps_clk_src is %s\n", clk_is_enabled(hw) ? "ENABLED" : "DISABLED");
>>
>> to drivers/clk/qcom/clk-rcg2.c : __clk_rcg2_set_rate()
>>
>>
> 
> Ok. You're reading the software state because there isn't an is_enabled
> clk_op for RCGs. Can you also read the CMD register (0x0 offset from
> base) and check for CMD_ROOT_EN (bit 1) being set? That's what I mean
> when I'm talking about the RCG being enabled in hardware. Similarly,
> read CMD_ROOT_OFF (bit 31) to see if some child branch of the RCG is
> enabled at this time.

[    3.998362] gcc_sdcc2_apps_clk_src is SW-DISABLED, CMD_ROOT_EN=0 CMD_ROOT_OFF=1
[    3.999896] scsi host0: ufshcd
[    4.006712] ------------[ cut here ]------------
[    4.013751] gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.

[...]

[    4.288626] gcc_sdcc2_apps_clk_src is SW-ENABLED, CMD_ROOT_EN=0 CMD_ROOT_OFF=0


Code:

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 9b3aaa7f20ac..a24b8931d7a1 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -471,6 +471,12 @@ static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate,
        struct clk_rcg2 *rcg = to_clk_rcg2(hw);
        const struct freq_tbl *f;
 
+       if (rcg->cmd_rcgr == 0x24014)
+               pr_err("gcc_sdcc2_apps_clk_src is SW-%s, CMD_ROOT_EN=%u CMD_ROOT_OFF=%u\n",
+                      clk_hw_is_enabled(hw) ? "ENABLED" : "DISABLED",
+                      regmap_test_bits(rcg->clkr.regmap, 0x24014 + CMD_REG, CMD_ROOT_EN),
+                      regmap_test_bits(rcg->clkr.regmap, 0x24014 + CMD_REG, CMD_ROOT_OFF));
+
        switch (policy) {
        case FLOOR:
                f = qcom_find_freq_floor(rcg->freq_tbl, rate);

Konrad
Konrad Dybcio June 8, 2024, 12:10 a.m. UTC | #9 
On 6.06.2024 1:56 PM, Konrad Dybcio wrote:
> On 7.05.2024 11:52 PM, Stephen Boyd wrote:
>> Quoting Konrad Dybcio (2024-05-07 14:17:01)
>>>
>>>
>>> On 5/7/24 22:28, Stephen Boyd wrote:
>>>>>
>>>>
>>>> Can you share your patch that prints the message? What bit are you
>>>> checking in the hardware to determine if the RCG is enabled? Do you also
>>>> print the enable count in software?
>>>
>>> I already reset-ed the tree state, but I added something like
>>>
>>> if (rcg->cmd_rcgr == the one in the declaration)
>>>         pr_err("gcc_sdcc2_apps_clk_src is %s\n", clk_is_enabled(hw) ? "ENABLED" : "DISABLED");
>>>
>>> to drivers/clk/qcom/clk-rcg2.c : __clk_rcg2_set_rate()
>>>
>>>
>>
>> Ok. You're reading the software state because there isn't an is_enabled
>> clk_op for RCGs. Can you also read the CMD register (0x0 offset from
>> base) and check for CMD_ROOT_EN (bit 1) being set? That's what I mean
>> when I'm talking about the RCG being enabled in hardware. Similarly,
>> read CMD_ROOT_OFF (bit 31) to see if some child branch of the RCG is
>> enabled at this time.
> 
> [    3.998362] gcc_sdcc2_apps_clk_src is SW-DISABLED, CMD_ROOT_EN=0 CMD_ROOT_OFF=1
> [    3.999896] scsi host0: ufshcd
> [    4.006712] ------------[ cut here ]------------
> [    4.013751] gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
> 
> [...]
> 
> [    4.288626] gcc_sdcc2_apps_clk_src is SW-ENABLED, CMD_ROOT_EN=0 CMD_ROOT_OFF=0
> 

Err.. one more thing.. After removing the HW_CTL logic that I introduced in
Commit a0e0ec7424c9 ("clk: qcom: rcg2: Make hw_clk_ctrl toggleable"), this
warn goes away.. I suppose I was already asked whether it's actually necessary
to drop it, and IIRC I shoved it in with my GPU enablement.. I'll retest whether
it's actually necessary, but I don't think so.

Still, doesn't explain why we needed this flag on so many other SoCs where
HW_CTL was left unset

Konrad
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
index e86c58bc5e48..9a1d48ff22bc 100644
--- a/drivers/clk/qcom/gcc-sm8450.c
+++ b/drivers/clk/qcom/gcc-sm8450.c
@@ -935,7 +935,7 @@  static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
 		.name = "gcc_sdcc2_apps_clk_src",
 		.parent_data = gcc_parent_data_7,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_7),
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 		.ops = &clk_rcg2_floor_ops,
 	},
 };