LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Michael Turquette <mturquette@baylibre.com>,
	Taniya Das <tdas@codeaurora.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	chandanu@codeaurora.org, Taniya Das <tdas@codeaurora.org>
Subject: Re: [PATCH v1 1/2] clk: qcom: rcg2: Add support for display port clock ops
Date: Tue, 09 Oct 2018 13:46:38 -0700	[thread overview]
Message-ID: <153911799897.119890.15102407227774253910@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <1539093467-12123-2-git-send-email-tdas@codeaurora.org>

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

  reply	other threads:[~2018-10-09 20:46 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-09 13:57 [PATCH v1 0/2] Add support for display port clocks and " Taniya Das
2018-10-09 13:57 ` [PATCH v1 1/2] clk: qcom: rcg2: Add support for display port " Taniya Das
2018-10-09 20:46   ` Stephen Boyd [this message]
2018-10-19 10:31     ` Taniya Das
2018-10-09 13:57 ` [PATCH v1 2/2] clk: qcom : dispcc: Add support for display port clocks Taniya Das
2018-10-09 20:34   ` Stephen Boyd
2018-10-19 10:34     ` Taniya Das
2018-10-28 10:34       ` Taniya Das
2018-10-29 18:43         ` Stephen Boyd
2018-10-30  6:01           ` Taniya Das
2018-10-30 16:33             ` Stephen Boyd
2018-11-01  5:02               ` Taniya Das
2018-11-06 17:08                 ` Stephen Boyd
2018-11-12  3:41                   ` chandanu
2019-02-02  0:05           ` chandanu
2019-07-15 22:59             ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=153911799897.119890.15102407227774253910@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=andy.gross@linaro.org \
    --cc=chandanu@codeaurora.org \
    --cc=david.brown@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=rnayak@codeaurora.org \
    --cc=tdas@codeaurora.org \
    --subject='Re: [PATCH v1 1/2] clk: qcom: rcg2: Add support for display port clock ops' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).