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

[Taniya Das]

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.

--