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

[Stephen Boyd]

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.