Linux-Clk Archive on lore.kernel.org
 help / color / 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
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 index

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 publically 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 \
    /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

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox