Hello Stephen, Thanks for your review. On 7/16/2019 4:13 AM, Stephen Boyd wrote:
Quoting Taniya Das (2019-05-14 21:20:38)diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index 18bdf34..0de080f 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -15,6 +15,7 @@ menuconfig COMMON_CLK_QCOM depends on ARCH_QCOM || COMPILE_TEST select REGMAP_MMIO select RESET_CONTROLLER + select RATIONALMake this an alphabetical list of selects please.
Sure, would take care in the next patch.
if COMMON_CLK_QCOM diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index 8c02bff..98071c0 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -1128,3 +1129,81 @@ int qcom_cc_register_rcg_dfs(struct regmap *regmap, return 0; } EXPORT_SYMBOL_GPL(qcom_cc_register_rcg_dfs); + +static int clk_rcg2_dp_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_rcg2 *rcg = to_clk_rcg2(hw); + struct freq_tbl f = { 0 }; + u32 mask = BIT(rcg->hid_width) - 1; + u32 hid_div, cfg; + int i, num_parents = clk_hw_get_num_parents(hw); + unsigned long num, den; + + rational_best_approximation(parent_rate, rate, + GENMASK(rcg->mnd_width - 1, 0), + GENMASK(rcg->mnd_width - 1, 0), &den, &num); + + if (!num || !den) { + pr_err("Invalid MN values derived for requested rate %lu\n",Does this ever happen? I worry that this printk could happen many times if a driver gets into a bad state and starts selecting invalid frequencies over and over again for each frame (every 16ms). Maybe just return -EINVAL instead of printing anything.
Would remove the pr_err.
+ rate); + return -EINVAL; + } + + regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); + hid_div = cfg; + cfg &= CFG_SRC_SEL_MASK; + cfg >>= CFG_SRC_SEL_SHIFT; + + for (i = 0; i < num_parents; i++) + if (cfg == rcg->parent_map[i].cfg) { + f.src = rcg->parent_map[i].src; + break; + }Weird indent for this brace. Please fix and put a brace on the for statement too.
My bad would fix in the next patch.
+ + f.pre_div = hid_div; + f.pre_div >>= CFG_SRC_DIV_SHIFT; + f.pre_div &= mask; + + if (num == den) { + f.m = 0; + f.n = 0;Isn't this the default? So just have if (num != den) here.
I would check for (num != den).
+ } else { + f.m = num; + f.n = den; + } + + return clk_rcg2_configure(rcg, &f); +} + +static int clk_rcg2_dp_set_rate_and_parent(struct clk_hw *hw, + unsigned long rate, unsigned long parent_rate, u8 index) +{ + return clk_rcg2_dp_set_rate(hw, rate, parent_rate); +}Does this need to be implemented? The parent index isn't passed to clk_rcg2_dp_set_rate() so I suspect the parent index doesn't matter? Does the parent change?
I guess it is required as the RCG SRC would be 0x0 by default.
+ +static int clk_rcg2_dp_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + struct clk_rate_request parent_req = *req; + int ret; + + ret = __clk_determine_rate(clk_hw_get_parent(hw), &parent_req); + if (ret) + return ret; + + req->best_parent_rate = parent_req.rate; + + return 0; +}Do you need this op? It's just calling determine rate on the parent, so we already do that if the proper flag is set. I'm confused about this function.
Would it be good to leave this function :). -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
