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

[Stephen Boyd]

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.

>  #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.

> +       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?

> +
> +       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;
> +}
> +
> +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);