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

Taniya Das
2019-07-31T18:15:09Z

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.

--