diff mbox series

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

Message ID 1539093467-12123-2-git-send-email-tdas@codeaurora.org (mailing list archive)
State Changes Requested, archived
Headers show
Series Add support for display port clocks and clock ops | expand

Commit Message

Taniya Das Oct. 9, 2018, 1:57 p.m. UTC 
New display port clock ops supported for display port clocks.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/clk-rcg.h  |  1 +
 drivers/clk/qcom/clk-rcg2.c | 86 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Comments

Stephen Boyd Oct. 9, 2018, 8:46 p.m. UTC | #1 
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);
Taniya Das Oct. 19, 2018, 10:31 a.m. UTC | #2 
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);
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index e5eca8a..e0db0f7 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -162,6 +162,7 @@  struct clk_rcg2 {
 extern const struct clk_ops clk_pixel_ops;
 extern const struct clk_ops clk_gfx3d_ops;
 extern const struct clk_ops clk_rcg2_shared_ops;
+extern const struct clk_ops clk_dp_ops;

 struct clk_rcg_dfs_data {
 	struct clk_rcg2 *rcg;
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>
 #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));
+	if (src_rate <= 0) {
+		pr_err("Invalid RCG parent rate\n");
+		return -EINVAL;
+	}
+
+	rational_best_approximation(src_rate, rate,
+			(unsigned long)(1 << 16) - 1,
+			(unsigned long)(1 << 16) - 1, &den, &num);
+
+	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;
+	}
+
+	req->best_parent_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
+	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);