lkml.org 
[lkml]   [2018]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 1/2] clk: qcom: rcg2: Add support for display port clock ops
From
Date
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.

--

\
 
 \ /
  Last update: 2018-10-19 12:32    [W:0.464 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site