Currently mailbox controller takes the XO and APSS PLL as the input. It can take the GPLL0 also as an input. This patch series adds the same and fixes the issue caused by this.
Once the cpufreq driver is up, it tries to bump up the cpu frequency above 800MHz, while doing so system is going to unusable state. Reason being, with the GPLL0 included as clock source, clock framework tries to achieve the required rate with the possible parent and since GPLL0 carries the CLK_SET_RATE_PARENT flag, clock rate of the GPLL0 is getting changed, causing the issue.
First half of the series, removes the CLK_SET_RATE_PARENT flag from the PLL clocks since the PLL clock rates shouldn't be changed. Another half, add the necessary support to include the GPLL0 as clock provider for mailbox and accomodate the changes in APSS clock driver.
This is also the preparatory series to enable the CPUFreq on IPQ5332 SoC. Dynamic scaling of CPUFreq is not supported on IPQ5332, so to switch between the frequencies we need to park the APSS PLL in safe source, here it is GPLL0 and then shutdown and bring up the APSS PLL in the desired rate.
For IPQ5332 SoC, this series depends on the below patch https://lore.kernel.org/linux-arm-msm/1693474133-10467-1-git-send-email-quic...
Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com --- Changes in v2: - included the patch to drop the CLK_SET_RATE_PARENT from IPQ5018 GCC driver - Splitted the DTS changes per target - For IPQ8074 and IPQ6018 keep the CLK_SET_RATE_PARENT for UBI32 PLL since the PLL clock rates can be changed - Pick up the tags in the relevant patches - Link to v1: https://lore.kernel.org/r/20230904-gpll_cleanup-v1-0-de2c448f1188@quicinc.co...
--- Kathiravan Thirumoorthy (11): clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks dt-bindings: mailbox: qcom: add one more clock provider for IPQ mailbox clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox
.../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml | 2 ++ arch/arm64/boot/dts/qcom/ipq5332.dtsi | 4 ++-- arch/arm64/boot/dts/qcom/ipq6018.dtsi | 4 ++-- arch/arm64/boot/dts/qcom/ipq8074.dtsi | 4 ++-- arch/arm64/boot/dts/qcom/ipq9574.dtsi | 4 ++-- drivers/clk/qcom/apss-ipq6018.c | 3 +++ drivers/clk/qcom/gcc-ipq5018.c | 3 --- drivers/clk/qcom/gcc-ipq5332.c | 2 -- drivers/clk/qcom/gcc-ipq6018.c | 6 ------ drivers/clk/qcom/gcc-ipq8074.c | 6 ------ drivers/clk/qcom/gcc-ipq9574.c | 4 ---- 11 files changed, 13 insertions(+), 29 deletions(-) --- base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56 change-id: 20230913-gpll_cleanup-5d0a339ebd17
Best regards,
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com --- Changes in V2: - Include the stable mailing list - Keep the CLK_SET_RATE_PARENT in UBI32 PLL, looks like these PLL rates can be changed. So don't drop the flag. --- drivers/clk/qcom/gcc-ipq8074.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/clk/qcom/gcc-ipq8074.c b/drivers/clk/qcom/gcc-ipq8074.c index 63ac2ced76bb..b7faf12a511a 100644 --- a/drivers/clk/qcom/gcc-ipq8074.c +++ b/drivers/clk/qcom/gcc-ipq8074.c @@ -75,7 +75,6 @@ static struct clk_fixed_factor gpll0_out_main_div2 = { &gpll0_main.clkr.hw }, .num_parents = 1, .ops = &clk_fixed_factor_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -121,7 +120,6 @@ static struct clk_alpha_pll_postdiv gpll2 = { &gpll2_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -154,7 +152,6 @@ static struct clk_alpha_pll_postdiv gpll4 = { &gpll4_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -188,7 +185,6 @@ static struct clk_alpha_pll_postdiv gpll6 = { &gpll6_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -201,7 +197,6 @@ static struct clk_fixed_factor gpll6_out_main_div2 = { &gpll6_main.clkr.hw }, .num_parents = 1, .ops = &clk_fixed_factor_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -266,7 +261,6 @@ static struct clk_alpha_pll_postdiv nss_crypto_pll = { &nss_crypto_pll_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com
Stephen, do you think there should be some sort of error or at least warning thrown when SET_RATE_PARENT is used with RO ops?
Konrad
Quoting Konrad Dybcio (2023-09-15 05:19:56)
On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com
Stephen, do you think there should be some sort of error or at least warning thrown when SET_RATE_PARENT is used with RO ops?
Sure? How would that be implemented?
On 10/19/23 02:16, Stephen Boyd wrote:
Quoting Konrad Dybcio (2023-09-15 05:19:56)
On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com
Stephen, do you think there should be some sort of error or at least warning thrown when SET_RATE_PARENT is used with RO ops?
Sure? How would that be implemented?
drivers/clk/clk.c : static void clk_change_rate()
if (!skip_set_rate && core->ops->set_rate) core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
->
if (!skip_set_rate) { if (core->ops->set_rate) core->ops->set_rate(core->hw, core->new_rate, best_parent_rate); else pr_err("bad idea"); }
Konrad
Quoting Konrad Dybcio (2023-10-19 04:22:33)
On 10/19/23 02:16, Stephen Boyd wrote:
Quoting Konrad Dybcio (2023-09-15 05:19:56)
On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com
Stephen, do you think there should be some sort of error or at least warning thrown when SET_RATE_PARENT is used with RO ops?
Sure? How would that be implemented?
drivers/clk/clk.c : static void clk_change_rate()
if (!skip_set_rate && core->ops->set_rate) core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
->
if (!skip_set_rate) { if (core->ops->set_rate) core->ops->set_rate(core->hw, core->new_rate, best_parent_rate); else pr_err("bad idea"); }
CLK_SET_RATE_PARENT means that "calling clk_set_rate() on this clk will propagate up to the parent". Changing the rate of the parent could change the rate of this clk to be the same frequency as the parent if this clk doesn't have a set_rate clk op, or it could be that this clk has a fixed divider so during the determine_rate() callback it calculated what rate the parent should be to achieve the requested rate in clk_set_rate().
It really matters what determine_rate() returns for a clk and if after changing rates that rate is actually achieved. I suppose if the determine_rate() callback returns some rate, and then we recalc rates and notice that the rate determined earlier doesn't match we're concerned. So far in the last decade we've never cared about this though and I'm hesitant to start adding that check. I believe some qcom clk drivers take a shortcut and round the rate in frequency tables so whatever is returned in determine_rate() doesn't match what recalc_rate() calculates.
It would be interesting to get rid of the CLK_SET_RATE_PARENT check in clk_calc_new_rates() and simply always call clk_calc_new_rates() on the parent if the parent->rate doesn't match what determine_rate thought it should be. The framework currently calls the rounding clk op for a clk and gets back the parent rate that the clk requires to achieve that rate and then it blindly trusts that the parent rate is going to be achieved. If the CLK_SET_RATE_PARENT flag is set it calls clk_calc_new_rates() recursively on the parent, but then it doesn't check that the parent rate is what was requested. That's mostly there to figure out if the parent also needs to change rate, i.e. calculating the 'top' clk in a rate change. Note that this also calls determine_rate again on the parent, once from the child clk's determine_rate clk op and once from the framework.
I wouldn't be surprised if some driver is relying on this behavior where the rate isn't checked after being set. Maybe when we extend struct clk_rate_request to have a linked list that allows a clk to build up a chain of rate requests we can also enforce more things like matching rates on recalc. Then any drivers that are relying on this behavior will have to opt in to a different method of changing rates and notice that things aren't working.
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: d9db07f088af ("clk: qcom: Add ipq6018 Global Clock Controller support") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com --- Changes in V2: - Include the stable mailing list - Keep the CLK_SET_RATE_PARENT in UBI32 PLL, looks like these PLL rates can be changed. So don't drop the flag. --- drivers/clk/qcom/gcc-ipq6018.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c index 6120fbbc5de0..f9494fa1b871 100644 --- a/drivers/clk/qcom/gcc-ipq6018.c +++ b/drivers/clk/qcom/gcc-ipq6018.c @@ -72,7 +72,6 @@ static struct clk_fixed_factor gpll0_out_main_div2 = { &gpll0_main.clkr.hw }, .num_parents = 1, .ops = &clk_fixed_factor_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -86,7 +85,6 @@ static struct clk_alpha_pll_postdiv gpll0 = { &gpll0_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -161,7 +159,6 @@ static struct clk_alpha_pll_postdiv gpll6 = { &gpll6_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -192,7 +189,6 @@ static struct clk_alpha_pll_postdiv gpll4 = { &gpll4_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -243,7 +239,6 @@ static struct clk_alpha_pll_postdiv gpll2 = { &gpll2_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
@@ -274,7 +269,6 @@ static struct clk_alpha_pll_postdiv nss_crypto_pll = { &nss_crypto_pll_main.clkr.hw }, .num_parents = 1, .ops = &clk_alpha_pll_postdiv_ro_ops, - .flags = CLK_SET_RATE_PARENT, }, };
On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based on the request from dependent clocks. Doing so will result in the unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL clocks.
Cc: stable@vger.kernel.org Fixes: d9db07f088af ("clk: qcom: Add ipq6018 Global Clock Controller support") Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com
Reviewed-by: Konrad Dybcio konrad.dybcio@linaro.org
Konrad
On 9/14/2023 12:29 PM, Kathiravan Thirumoorthy wrote:
Currently mailbox controller takes the XO and APSS PLL as the input. It can take the GPLL0 also as an input. This patch series adds the same and fixes the issue caused by this.
Once the cpufreq driver is up, it tries to bump up the cpu frequency above 800MHz, while doing so system is going to unusable state. Reason being, with the GPLL0 included as clock source, clock framework tries to achieve the required rate with the possible parent and since GPLL0 carries the CLK_SET_RATE_PARENT flag, clock rate of the GPLL0 is getting changed, causing the issue.
First half of the series, removes the CLK_SET_RATE_PARENT flag from the PLL clocks since the PLL clock rates shouldn't be changed. Another half, add the necessary support to include the GPLL0 as clock provider for mailbox and accomodate the changes in APSS clock driver.
This is also the preparatory series to enable the CPUFreq on IPQ5332 SoC. Dynamic scaling of CPUFreq is not supported on IPQ5332, so to switch between the frequencies we need to park the APSS PLL in safe source, here it is GPLL0 and then shutdown and bring up the APSS PLL in the desired rate.
For IPQ5332 SoC, this series depends on the below patch https://lore.kernel.org/linux-arm-msm/1693474133-10467-1-git-send-email-quic...
Bjorn, can this series picked up for v6.7?
There is a minor nit the commit message. The statement "APSS PLL will be running at 800MHz" should be "APSS clock / CPU clock will be running at 800MHz" and this should be taken care for clk and the dts patches. Do let me know if I need to re-spin the address to address this.
Thanks,
Signed-off-by: Kathiravan Thirumoorthy quic_kathirav@quicinc.com
Changes in v2:
- included the patch to drop the CLK_SET_RATE_PARENT from IPQ5018 GCC driver
- Splitted the DTS changes per target
- For IPQ8074 and IPQ6018 keep the CLK_SET_RATE_PARENT for UBI32 PLL since the PLL clock rates can be changed
- Pick up the tags in the relevant patches
- Link to v1: https://lore.kernel.org/r/20230904-gpll_cleanup-v1-0-de2c448f1188@quicinc.co...
Kathiravan Thirumoorthy (11): clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks dt-bindings: mailbox: qcom: add one more clock provider for IPQ mailbox clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox
.../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml | 2 ++ arch/arm64/boot/dts/qcom/ipq5332.dtsi | 4 ++-- arch/arm64/boot/dts/qcom/ipq6018.dtsi | 4 ++-- arch/arm64/boot/dts/qcom/ipq8074.dtsi | 4 ++-- arch/arm64/boot/dts/qcom/ipq9574.dtsi | 4 ++-- drivers/clk/qcom/apss-ipq6018.c | 3 +++ drivers/clk/qcom/gcc-ipq5018.c | 3 --- drivers/clk/qcom/gcc-ipq5332.c | 2 -- drivers/clk/qcom/gcc-ipq6018.c | 6 ------ drivers/clk/qcom/gcc-ipq8074.c | 6 ------ drivers/clk/qcom/gcc-ipq9574.c | 4 ---- 11 files changed, 13 insertions(+), 29 deletions(-)
base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56 change-id: 20230913-gpll_cleanup-5d0a339ebd17
Best regards,
On Thu, 14 Sep 2023 12:29:50 +0530, Kathiravan Thirumoorthy wrote:
Currently mailbox controller takes the XO and APSS PLL as the input. It can take the GPLL0 also as an input. This patch series adds the same and fixes the issue caused by this.
Once the cpufreq driver is up, it tries to bump up the cpu frequency above 800MHz, while doing so system is going to unusable state. Reason being, with the GPLL0 included as clock source, clock framework tries to achieve the required rate with the possible parent and since GPLL0 carries the CLK_SET_RATE_PARENT flag, clock rate of the GPLL0 is getting changed, causing the issue.
[...]
Applied, thanks!
[01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks commit: e641a070137dd959932c7c222e000d9d941167a2 [02/11] clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks commit: 99cd4935cb972d0aafb16838bb2aeadbcaf196ce [03/11] clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks commit: 01a5e4c6731ab6b4b74822661d296f8893fc1230 [04/11] clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks commit: 99a8f8764b70158a712992640a6be46a8fd79d15 [05/11] clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks commit: 5635ef0bd1052420bc659a00be6fd0c60cec5cb9 [07/11] clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider commit: e0e6373d653b7707bf042ecf1538884597c5d0da [08/11] arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox commit: 80ebe63329909531afc87335f1d95c7bf8414438 [09/11] arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox commit: 0133c7af3aa0420778d106cb90db708cfa45f2c6 [10/11] arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox commit: 77c726a4f3b124903db5ced7d597976d5b80dcfb [11/11] arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox commit: da528016952bf93ca810c43fafe518c699db7fa0
Best regards,
linux-stable-mirror@lists.linaro.org
-
Bjorn Andersson -
Kathiravan Thirumoorthy -
Konrad Dybcio -
Stephen Boyd