| Message ID | 20230117135459.16868-5-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: clk-rcg2: introduce support for multiple conf for same freq | expand |
Quoting Christian Marangi (2023-01-17 05:54:57) > Currently the rcg2 driver search the rate to apply 2 times. > - In _freq_tbl_determine_rate for the determine_rate function used by > core clk to understand the best rate to set with set_rate > - In rcg2_set_rate where the suggested rate is not trusted and searched > another time using a CEIL or FLOOR policy. > > This is fundamentally wrong as we are ignoring what core clock is > deciding and just setting whatever clock configuration we want for the > suggested clock. The problem is in the fact that the correct clock > should have already be searched and selected with the determine_rate > function and set_rate should just apply whatever clock was provided. It sounds like you're assuming the rate coming into the set_rate clk_op is rounded? Don't make that assumption. The set_rate clk_op should round the rate again. The parent rate could have changed.
On 29/03/2023 22:52, Stephen Boyd wrote: > Quoting Christian Marangi (2023-01-17 05:54:57) >> Currently the rcg2 driver search the rate to apply 2 times. >> - In _freq_tbl_determine_rate for the determine_rate function used by >> core clk to understand the best rate to set with set_rate >> - In rcg2_set_rate where the suggested rate is not trusted and searched >> another time using a CEIL or FLOOR policy. >> >> This is fundamentally wrong as we are ignoring what core clock is >> deciding and just setting whatever clock configuration we want for the >> suggested clock. The problem is in the fact that the correct clock >> should have already be searched and selected with the determine_rate >> function and set_rate should just apply whatever clock was provided. > > It sounds like you're assuming the rate coming into the set_rate clk_op > is rounded? Don't make that assumption. The set_rate clk_op should round > the rate again. The parent rate could have changed. My fault, I suggested this some time ago on the basis that... "CCF switching the parent rate during the clk_set_rate() call. Then the second lookup might end up selecting different parent/mnd configuration."
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index 76551534f10d..3f15e993dc04 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -352,23 +352,12 @@ static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) return update_config(rcg); } -static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, - enum freq_policy policy) +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; - switch (policy) { - case FLOOR: - f = qcom_find_freq_floor(rcg->freq_tbl, rate); - break; - case CEIL: - f = qcom_find_freq(rcg->freq_tbl, rate); - break; - default: - return -EINVAL; - } - + f = qcom_find_freq_exact(rcg->freq_tbl, rate); if (!f) return -EINVAL; @@ -378,25 +367,25 @@ static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, static int clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { - return __clk_rcg2_set_rate(hw, rate, CEIL); + return __clk_rcg2_set_rate(hw, rate); } static int clk_rcg2_set_floor_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { - return __clk_rcg2_set_rate(hw, rate, FLOOR); + return __clk_rcg2_set_rate(hw, rate); } static int clk_rcg2_set_rate_and_parent(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate, u8 index) { - return __clk_rcg2_set_rate(hw, rate, CEIL); + return __clk_rcg2_set_rate(hw, rate); } static int clk_rcg2_set_floor_rate_and_parent(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate, u8 index) { - return __clk_rcg2_set_rate(hw, rate, FLOOR); + return __clk_rcg2_set_rate(hw, rate); } static int clk_rcg2_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
Currently the rcg2 driver search the rate to apply 2 times. - In _freq_tbl_determine_rate for the determine_rate function used by core clk to understand the best rate to set with set_rate - In rcg2_set_rate where the suggested rate is not trusted and searched another time using a CEIL or FLOOR policy. This is fundamentally wrong as we are ignoring what core clock is deciding and just setting whatever clock configuration we want for the suggested clock. The problem is in the fact that the correct clock should have already be searched and selected with the determine_rate function and set_rate should just apply whatever clock was provided. Rework the function using the find_req_exact and by trusting whatever core clk is asking to set. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/clk/qcom/clk-rcg2.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)