Skip to contentSkip to conversation

clk: qcom: Add support for RCG to register for DFS

PATCH In review Taniya Das 3 participants 3 messages May 3, 2018 – May 8, 2018

Subsystems (3)
lore
Share & tools
lore

Taniya Das

author
#0 lore raw

PATCH 1/1 clk: qcom: Add support for RCG to register for DFS

In the cases where a RCG requires a Dynamic Frequency switch support requires to register which would at runtime read the clock perf level registers to identify the frequencies supported and update the frequency table accordingly.

---
 drivers/clk/qcom/clk-rcg.h  |   7 +-
 drivers/clk/qcom/clk-rcg2.c | 172 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/common.c   |  23 +++++-
 drivers/clk/qcom/common.h   |  14 +++-
 4 files changed, 213 insertions(+), 3 deletions(-)
drivers/clk/qcom/clk-rcg.h +6 -1
index 2a7489a..06de69f 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2013, 2018, The Linux Foundation. All rights reserved.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -144,6 +144,7 @@ struct clk_dyn_rcg {
  * @cmd_rcgr: corresponds to *_CMD_RCGR
  * @mnd_width: number of bits in m/n/d values
  * @hid_width: number of bits in half integer divider
+ * @flags: additional flag parameters for the RCG
  * @parent_map: map from software's parent index to hardware's src_sel field
  * @freq_tbl: frequency table
  * @clkr: regmap clock handle
@@ -153,6 +154,8 @@ struct clk_rcg2 {
 	u32			cmd_rcgr;
 	u8			mnd_width;
 	u8			hid_width;
+	u8			flags;
+#define DFS_ENABLE_RCG		BIT(0)
 	const struct parent_map	*parent_map;
 	const struct freq_tbl	*freq_tbl;
 	struct clk_regmap	clkr;
@@ -168,4 +171,6 @@ struct clk_rcg2 {
 extern const struct clk_ops clk_pixel_ops;
 extern const struct clk_ops clk_gfx3d_ops;
 
+extern int clk_rcg2_get_dfs_clock_rate(struct clk_rcg2 *clk,
+						struct device *dev);
 #endif
drivers/clk/qcom/clk-rcg2.c +172 -0

199 lines not on this page · raw message

drivers/clk/qcom/common.c +22 -1
index b8064a3..b27699b 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -288,4 +288,25 @@ int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc)
 }
 EXPORT_SYMBOL_GPL(qcom_cc_probe);
 
+int qcom_cc_register_rcg_dfs(struct platform_device *pdev,
+			 const struct qcom_cc_dfs_desc *desc)
+{
+	struct clk_dfs *clks = desc->clks;
+	size_t num_clks = desc->num_clks;
+	int i, ret = 0;
+
+	for (i = 0; i < num_clks; i++) {
+		ret = clk_rcg2_get_dfs_clock_rate(clks[i].rcg, &pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed calculating DFS frequencies for %s\n",
+				clk_hw_get_name(&(clks[i].rcg)->clkr.hw));
+			break;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(qcom_cc_register_rcg_dfs);
+
 MODULE_LICENSE("GPL v2");
drivers/clk/qcom/common.h +13 -1
index 00196ee..6627aec 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014, 2018, The Linux Foundation. All rights reserved.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -48,6 +48,15 @@ struct parent_map {
 	u8 cfg;
 };
 
+struct clk_dfs {
+	struct clk_rcg2 *rcg;
+};
+
+struct qcom_cc_dfs_desc {
+	struct clk_dfs *clks;
+	size_t num_clks;
+};
+
 extern const struct freq_tbl *qcom_find_freq(const struct freq_tbl *f,
 					     unsigned long rate);
 extern const struct freq_tbl *qcom_find_freq_floor(const struct freq_tbl *f,
@@ -69,4 +78,7 @@ extern int qcom_cc_really_probe(struct platform_device *pdev,
 extern int qcom_cc_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc);
 
+extern int qcom_cc_register_rcg_dfs(struct platform_device *pdev,
+			 const struct qcom_cc_dfs_desc *desc);
+
 #endif

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

#1 lore raw

Hi Taniya,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v4.17-rc3 next-20180503]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Taniya-Das/clk-qcom-Add-support-for-RCG-to-register-for-DFS/20180503-220112
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next

smatch warnings:
drivers/clk/qcom/clk-rcg2.c:820 clk_parent_index_pre_div_and_mode() warn: signedness bug returning '(-22)'

# https://github.com/0day-ci/linux/commit/ac73368dea3e89ee63025d4da64dd908bcde367e
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout ac73368dea3e89ee63025d4da64dd908bcde367e
vim +820 drivers/clk/qcom/clk-rcg2.c

ac73368d Taniya Das 2018-05-03  809  
ac73368d Taniya Das 2018-05-03  810  /* Common APIs to be used for DFS based RCGR */
ac73368d Taniya Das 2018-05-03  811  static u8 clk_parent_index_pre_div_and_mode(struct clk_hw *hw, u32 offset,
ac73368d Taniya Das 2018-05-03  812  		u32 *mode, u32 *pre_div)
ac73368d Taniya Das 2018-05-03  813  {
ac73368d Taniya Das 2018-05-03  814  	struct clk_rcg2 *rcg;
ac73368d Taniya Das 2018-05-03  815  	int num_parents;
ac73368d Taniya Das 2018-05-03  816  	u32 cfg, mask;
ac73368d Taniya Das 2018-05-03  817  	int i, ret;
ac73368d Taniya Das 2018-05-03  818  
ac73368d Taniya Das 2018-05-03  819  	if (!hw)
ac73368d Taniya Das 2018-05-03 @820  		return -EINVAL;
ac73368d Taniya Das 2018-05-03  821  
ac73368d Taniya Das 2018-05-03  822  	num_parents = clk_hw_get_num_parents(hw);
ac73368d Taniya Das 2018-05-03  823  
ac73368d Taniya Das 2018-05-03  824  	rcg = to_clk_rcg2(hw);
ac73368d Taniya Das 2018-05-03  825  
ac73368d Taniya Das 2018-05-03  826  	ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + offset, &cfg);
ac73368d Taniya Das 2018-05-03  827  	if (ret)
ac73368d Taniya Das 2018-05-03  828  		goto err;
ac73368d Taniya Das 2018-05-03  829  
ac73368d Taniya Das 2018-05-03  830  	mask = BIT(rcg->hid_width) - 1;
ac73368d Taniya Das 2018-05-03  831  	*pre_div = cfg & mask ? (cfg & mask) : 1;
ac73368d Taniya Das 2018-05-03  832  
ac73368d Taniya Das 2018-05-03  833  	*mode = cfg & CFG_MODE_MASK;
ac73368d Taniya Das 2018-05-03  834  	*mode >>= CFG_MODE_SHIFT;
ac73368d Taniya Das 2018-05-03  835  
ac73368d Taniya Das 2018-05-03  836  	cfg &= CFG_SRC_SEL_MASK;
ac73368d Taniya Das 2018-05-03  837  	cfg >>= CFG_SRC_SEL_SHIFT;
ac73368d Taniya Das 2018-05-03  838  
ac73368d Taniya Das 2018-05-03  839  	for (i = 0; i < num_parents; i++)
ac73368d Taniya Das 2018-05-03  840  		if (cfg == rcg->parent_map[i].cfg)
ac73368d Taniya Das 2018-05-03  841  			return i;
ac73368d Taniya Das 2018-05-03  842  err:
ac73368d Taniya Das 2018-05-03  843  	return 0;
ac73368d Taniya Das 2018-05-03  844  }
ac73368d Taniya Das 2018-05-03  845  
maintainer
#2 lore raw
In reply to Taniya Das

Quoting Taniya Das (2018-05-02 21:12:32)

Quoted 16 lines
drivers/clk/qcom/clk-rcg2.c +4 -0
index e63db10..7c35bca 100644
@@ -122,6 +131,10 @@ static int clk_rcg2_set_parent(struct clk_hw *hw, u8 index)
        int ret;
        u32 cfg = rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
 
+       /* In DFS mode skip updating the RCG CFG */
+       if (rcg->flags & DFS_ENABLE_RCG)
+               return 0;
+
        ret = regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG,
                                 CFG_SRC_SEL_MASK, cfg);
        if (ret)

drivers/clk/qcom/clk-rcg2.c static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate, · line 314

Quoted 6 lines
drivers/clk/qcom/clk-rcg2.c +3 -0
@@ -296,6 +309,9 @@ static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate,
        struct clk_rcg2 *rcg = to_clk_rcg2(hw);
        const struct freq_tbl *f;
 
+       if (rcg->flags & DFS_ENABLE_RCG)
+               return -EPERM;
+

Please no.

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 811

Quoted 10 lines
drivers/clk/qcom/clk-rcg2.c +3 -0
        switch (policy) {
        case FLOOR:
                f = qcom_find_freq_floor(rcg->freq_tbl, rate);
@@ -790,3 +806,159 @@ static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate,
        .determine_rate = clk_gfx3d_determine_rate,
 };
 EXPORT_SYMBOL_GPL(clk_gfx3d_ops);
+
+/* Common APIs to be used for DFS based RCGR */
+static u8 clk_parent_index_pre_div_and_mode(struct clk_hw *hw, u32 offset,

Fix return type please.

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 820

Quoted 9 lines
drivers/clk/qcom/clk-rcg2.c +9 -0
+               u32 *mode, u32 *pre_div)
+{
+       struct clk_rcg2 *rcg;
+       int num_parents;
+       u32 cfg, mask;
+       int i, ret;
+
+       if (!hw)
+               return -EINVAL;

Why?

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 826

Quoted 6 lines
drivers/clk/qcom/clk-rcg2.c +6 -0
+
+       num_parents = clk_hw_get_num_parents(hw);
+
+       rcg = to_clk_rcg2(hw);
+
+       ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + offset, &cfg);

We may need to pass the regmap into here so this can be done before the clk is registered so that we can switch out the clk_ops at runtime. But that may not be the case if my other comment makes sense further down.

Quoted 11 lines
drivers/clk/qcom/clk-rcg2.c +11 -0
+       if (ret)
+               goto err;
+
+       mask = BIT(rcg->hid_width) - 1;
+       *pre_div = cfg & mask ? (cfg & mask) : 1;
+
+       *mode = cfg & CFG_MODE_MASK;
+       *mode >>= CFG_MODE_SHIFT;
+
+       cfg &= CFG_SRC_SEL_MASK;
+       cfg >>= CFG_SRC_SEL_SHIFT;

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 847

Quoted 10 lines
drivers/clk/qcom/clk-rcg2.c +10 -0
+
+       for (i = 0; i < num_parents; i++)
+               if (cfg == rcg->parent_map[i].cfg)
+                       return i;
+err:
+       return 0;
+}
+
+static int calculate_m_and_n(struct clk_hw *hw, u32 m_offset, u32 n_offset,
+               u32 mode, u32 *m, u32 *n)

Instead of returning pointers, please have these functions update a frequency table entry structure.

Quoted 40 lines
drivers/clk/qcom/clk-rcg2.c +40 -0
+{
+       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+       u32 val, mask;
+       int ret = 0;
+
+       if (!hw)
+               return -EINVAL;
+
+       *m = *n = 0;
+
+       if (mode) {
+               /* Calculate M & N values */
+               mask = BIT(rcg->mnd_width) - 1;
+               ret =  regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + m_offset,
+                                       &val);
+               if (ret) {
+                       pr_err("Failed to read M offset register\n");
+                       goto err;
+               }
+
+               val &= mask;
+               *m  = val;
+
+               ret =  regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + n_offset,
+                                       &val);
+               if (ret) {
+                       pr_err("Failed to read N offset register\n");
+                       goto err;
+               }
+
+               /* val ~(N-M) */
+               val = ~val;
+               val &= mask;
+               val += *m;
+               *n = val;
+       }
+err:
+       return ret;
+}
+

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 897

Quoted 10 lines
drivers/clk/qcom/clk-rcg2.c +10 -0
+int clk_rcg2_get_dfs_clock_rate(struct clk_rcg2 *clk, struct device *dev)
+{
+       int i, j, index, ret = 0;
+       unsigned long calc_freq, prate;
+       u32 val, pre_div = 0, mode = 0, m = 0, n = 0;
+       struct freq_tbl *dfs_freq_tbl;
+       struct clk_hw *phw;
+
+       if (!clk)
+               return -EINVAL;

Never happens?

Quoted 4 lines
drivers/clk/qcom/clk-rcg2.c +4 -0
+
+       /* Check for DFS_EN */
+       ret = regmap_read(clk->clkr.regmap, clk->cmd_rcgr + SE_CMD_DFSR_OFFSET,
+                                               &val);

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 911

Quoted 10 lines
drivers/clk/qcom/clk-rcg2.c +10 -0
+       if (ret) {
+               dev_err(dev, "Failed to read DFS enable register\n");
+               return -EINVAL;
+       }
+
+       if (!(val & SE_CMD_DFS_EN))
+               return ret;
+
+       dfs_freq_tbl = devm_kzalloc(dev, MAX_PERF_LEVEL *
+                               sizeof(struct freq_tbl), GFP_KERNEL);

Use devm_kcalloc() for numbers of elements in an array.

Quoted 13 lines
drivers/clk/qcom/clk-rcg2.c +13 -0
+       if (!dfs_freq_tbl)
+               return -ENOMEM;
+
+       /* Populate the Perf Level */
+       for (i = 0; i < MAX_PERF_LEVEL; i++) {
+               /* Get parent index and mode */
+               index = clk_parent_index_pre_div_and_mode(&clk->clkr.hw,
+                                                       SE_PERF_DFSR(i), &mode,
+                                                       &pre_div);
+               if (index < 0) {
+                       pr_err("Failed to get parent index & mode %d\n", index);
+                       return index;
+               }

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 934

Quoted 10 lines
drivers/clk/qcom/clk-rcg2.c +10 -0
+
+               /* clock pre_div */
+               dfs_freq_tbl[i].pre_div = pre_div;
+
+               /* Fill the parent src */
+               dfs_freq_tbl[i].src = clk->parent_map[index].src;
+
+               /* Get the parent clock and parent rate */
+               phw = clk_hw_get_parent_by_index(&clk->clkr.hw, index);
+               prate = clk_hw_get_rate(phw);

Urgh this is sort of annoying. We'll need the parents to be registered when the frequency table is created, just so we can parse the parent rate out and calculate the final frequency? What do we do if the parent isn't registered yet? We may need to push this logic into the determine rate path, and calculate the frequency table lazily at that point and hope that the parent is registered with some frequency. Otherwise fail the determine rate call.

Quoted 14 lines
drivers/clk/qcom/clk-rcg2.c +14 -0
+
+               ret = calculate_m_and_n(&clk->clkr.hw, SE_PERF_M_DFSR(i),
+                                       SE_PERF_N_DFSR(i), mode, &m, &n);
+               if (ret)
+                       goto err;
+
+               dfs_freq_tbl[i].m = m;
+               dfs_freq_tbl[i].n = n;
+
+               /* calculate the final frequency */
+               calc_freq = calc_rate(prate, dfs_freq_tbl[i].m,
+                                               dfs_freq_tbl[i].n, mode,
+                                               dfs_freq_tbl[i].pre_div);
+

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 958

Quoted 10 lines
drivers/clk/qcom/clk-rcg2.c +10 -0
+               /* Check for duplicate frequencies */
+               for (j = 0; j  < i; j++) {
+                       if (dfs_freq_tbl[j].freq == calc_freq)
+                               goto done;
+               }
+
+               dfs_freq_tbl[i].freq = calc_freq;
+       }
+done:
+       j = i;

Why do we care about j = i? It isn't used now.

drivers/clk/qcom/clk-rcg2.c static int clk_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate, · line 960

Quoted 2 lines
drivers/clk/qcom/clk-rcg2.c +2 -0
+
+       clk->flags |= DFS_ENABLE_RCG;

I'd rather the ops are replaced with different clk_ops for DFS explicitly. The flag is not very nice.

Quoted 9 lines
drivers/clk/qcom/clk-rcg2.c +4 -0
+       clk->freq_tbl = dfs_freq_tbl;
+err:
+       return ret;
+}
drivers/clk/qcom/common.c +0 -0
index b8064a3..b27699b 100644

drivers/clk/qcom/common.c line 5

Quoted 6 lines
drivers/clk/qcom/common.c +1 -1
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and

SPDX update?

Quoted 7 lines
drivers/clk/qcom/common.c +3 -0
@@ -288,4 +288,25 @@ int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc)
 }
 EXPORT_SYMBOL_GPL(qcom_cc_probe);
 
+int qcom_cc_register_rcg_dfs(struct platform_device *pdev,
+                        const struct qcom_cc_dfs_desc *desc)
+{

drivers/clk/qcom/common.c int qcom_cc_probe(struct platform_device *pdev, const struct qcom_cc_desc *desc) · line 303

Quoted 10 lines
drivers/clk/qcom/common.c +10 -0
+       struct clk_dfs *clks = desc->clks;
+       size_t num_clks = desc->num_clks;
+       int i, ret = 0;
+
+       for (i = 0; i < num_clks; i++) {
+               ret = clk_rcg2_get_dfs_clock_rate(clks[i].rcg, &pdev->dev);
+               if (ret) {
+                       dev_err(&pdev->dev,
+                               "Failed calculating DFS frequencies for %s\n",
+                               clk_hw_get_name(&(clks[i].rcg)->clkr.hw));

That is a long line. Preferably iterate a local variable 'rcg' that is set to the start of the array and ++ through this loop.

Quoted 21 lines
drivers/clk/qcom/common.c +8 -0
+                       break;
+               }
+       }
+
+       return ret;
+}
+EXPORT_SYMBOL(qcom_cc_register_rcg_dfs);
+
 MODULE_LICENSE("GPL v2");
drivers/clk/qcom/common.h +1 -1
index 00196ee..6627aec 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014, 2018, The Linux Foundation. All rights reserved.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and

drivers/clk/qcom/common.h struct parent_map { · line 53

Quoted 6 lines
drivers/clk/qcom/common.h +3 -0
@@ -48,6 +48,15 @@ struct parent_map {
        u8 cfg;
 };
 
+struct clk_dfs {
+       struct clk_rcg2 *rcg;
+};

Just have an array of clk_rcg2's instead of this struct? I'd add it to the common descriptor structure too, because I'm going to guess this is common going forward.