| Message ID | 20230913-gpll_cleanup-v2-1-c8ceb1a37680@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add GPLL0 as clock provider for the Qualcomm's IPQ mailbox controller | expand |
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.
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, }, };
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(-)