Messages in this thread | | | Subject | Re: [PATCH v1 1/2] clk: qcom: rcg2: Add support for display port clock ops | From | Taniya Das <> | Date | Fri, 19 Oct 2018 16:01:35 +0530 |
| |
Hello Stephen,
On 10/10/2018 2:16 AM, Stephen Boyd wrote: > Quoting Taniya Das (2018-10-09 06:57:46) >> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c >> index 6e3bd19..ca6142f 100644 >> --- a/drivers/clk/qcom/clk-rcg2.c >> +++ b/drivers/clk/qcom/clk-rcg2.c >> @@ -10,6 +10,7 @@ >> #include <linux/export.h> >> #include <linux/clk-provider.h> >> #include <linux/delay.h> >> +#include <linux/rational.h> > > Can you also select RATIONAL in the Kconfig language? Yes the clk > subsystem is already selecting it, but it's nice to be explicit in the > subdrivers. >
Sure, would take care of adding it in KCONFIG.
>> #include <linux/regmap.h> >> #include <linux/math64.h> >> #include <linux/slab.h> >> @@ -1124,3 +1125,88 @@ 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 }; >> + unsigned long src_rate; >> + unsigned long num, den; >> + u32 mask = BIT(rcg->hid_width) - 1; >> + u32 hid_div, cfg; >> + int i, num_parents = clk_hw_get_num_parents(hw); >> + >> + src_rate = clk_hw_get_rate(clk_hw_get_parent(hw)); > > What is this doing? We shouldn't need to check the parent clk for any > rate here when we're setting the rate. >
Hmmm, let me get back on this.
>> + if (src_rate <= 0) { >> + pr_err("Invalid RCG parent rate\n"); >> + return -EINVAL; >> + } >> + >> + rational_best_approximation(src_rate, rate, >> + (unsigned long)(1 << 16) - 1, > > Use GENMASK? > >> + (unsigned long)(1 << 16) - 1, &den, &num); > > Same? >
Sure, would use GENMASK.
>> + >> + if (!num || !den) { >> + pr_err("Invalid MN values derived for requested rate %lu\n", >> + 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; >> + } >> + >> + 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; >> + } 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); >> +} >> + >> +static int clk_rcg2_dp_determine_rate(struct clk_hw *hw, >> + struct clk_rate_request *req) >> +{ >> + if (!hw) >> + return -EINVAL; >> + >> + if (!clk_hw_get_parent(hw)) { >> + pr_err("Missing the parent for the DP RCG\n"); >> + return -EINVAL; >> + } > > Let me Harry Potter this stuff. Expelliarmus! > >> + >> + req->best_parent_rate = clk_hw_get_rate(clk_hw_get_parent(hw)); > > Presumably we should ask the parent clk for the rate that is requested > here by calling determine rate up and see if the parent can do it. Sure, > this clk does nothing, so we don't really need any sort of op here then > and we can just flag the clk as CLK_SET_RATE_PARENT and let the core do > the rest. > >> + return 0; >> +} >> +
Would remove this.
>> +const struct clk_ops clk_dp_ops = { >> + .is_enabled = clk_rcg2_is_enabled, >> + .get_parent = clk_rcg2_get_parent, >> + .set_parent = clk_rcg2_set_parent, >> + .recalc_rate = clk_rcg2_recalc_rate, >> + .set_rate = clk_rcg2_dp_set_rate, >> + .set_rate_and_parent = clk_rcg2_dp_set_rate_and_parent, >> + .determine_rate = clk_rcg2_dp_determine_rate, >> +}; >> +EXPORT_SYMBOL_GPL(clk_dp_ops);
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation.
--
| |