| Message ID | 1557894039-31835-2-git-send-email-tdas@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | Add support for display port clocks and clock ops | expand |
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 RATIONAL Make this an alphabetical list of selects please. > > 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. > + 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. > + > + 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. > + } 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? > + > +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.
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 RATIONAL > > Make 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 :).
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 RATIONAL if COMMON_CLK_QCOM diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h index c25b57c..c6f64be 100644 --- a/drivers/clk/qcom/clk-rcg.h +++ b/drivers/clk/qcom/clk-rcg.h @@ -161,6 +161,7 @@ struct clk_rcg2 { extern const struct clk_ops clk_pixel_ops; extern const struct clk_ops clk_gfx3d_ops; extern const struct clk_ops clk_rcg2_shared_ops; +extern const struct clk_ops clk_dp_ops; struct clk_rcg_dfs_data { struct clk_rcg2 *rcg; 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 @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Copyright (c) 2013, 2018, The Linux Foundation. All rights reserved. + * Copyright (c) 2013, 2018-2019, The Linux Foundation. All rights reserved. */ #include <linux/kernel.h> @@ -10,6 +10,7 @@ #include <linux/export.h> #include <linux/clk-provider.h> #include <linux/delay.h> +#include <linux/rational.h> #include <linux/regmap.h> #include <linux/math64.h> #include <linux/slab.h> @@ -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", + 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) +{ + 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; +} + +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);
New display port clock ops supported for display port clocks. Signed-off-by: Taniya Das <tdas@codeaurora.org> --- drivers/clk/qcom/Kconfig | 1 + drivers/clk/qcom/clk-rcg.h | 1 + drivers/clk/qcom/clk-rcg2.c | 81 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 82 insertions(+), 1 deletion(-) -- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.