Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Taniya Das <tdas@codeaurora.org>
To: Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>
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
Subject: Re: [PATCH v1 1/2] clk: qcom: rcg2: Add support for display port clock ops
Date: Fri, 19 Oct 2018 16:01:35 +0530
Message-ID: <02289ba9-a528-02e8-df25-892ccfd0f9bf@codeaurora.org> (raw)
In-Reply-To: <153911799897.119890.15102407227774253910@swboyd.mtv.corp.google.com>

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.

--

  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
2018-10-19 10:31     ` Taniya Das [this message]
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=02289ba9-a528-02e8-df25-892ccfd0f9bf@codeaurora.org \
    --to=tdas@codeaurora.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=sboyd@kernel.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