Re: [PATCH v2 1/2] clk: qcom: rcg2: Add support for display port clock ops

[Taniya Das <tdas@codeaurora.org>]

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 :).

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--