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

[Taniya Das <tdas@codeaurora.org>]

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.

--