[PATCH v2 0/2] clk: qcom: Add support for RCG to register for DFS

[PATCH v2 0/2] clk: qcom: Add support for RCG to register for DFS

[Taniya Das]

 [v2]
  * Move the dfs register function 'qcom_cc_register_rcg_dfs'
    to clk-rcg2.c instead of common.c
  * At boot read the DFS enable register and override the clk_ops
    to be used for dfs or non-dfs RCGs.
  * Remove flag 'dfs_enabled'.
  * Remove functions 'clk_rcg2_dfs_determine_rate_lazy'
  * Remove 'struct dfs_table *dfs_entry'
  * Remove '_freq_tbl_determine_dfs_rate'
  * Combine the function 'clk_index_pre_div_and_mode' and 'calculate_m_and_n'
    to a single function and named it 'clk_rcg2_calculate_m_and_n'.
  * Remove taking M/N/PERF offsets as function arguments.
  * Add clocks in gcc-sdm845.c the DFS clock array to register.

 [v1]
   * Update SPDX for files.
   * Add new clk_ops for DFS mode which would be used if dfs is enabled,
     else fall back to the clk_rcg2_shared_ops.
   * Use kcalloc in place kzalloc.
   * Fixed the return type for 'clk_parent_index_pre_div_and_mode' which
     is now renamed to 'clk_index_pre_div_and_mode'.
   * Removed return of -EPERM from 'clk_rcg2_set_rate' and new dfs
     clk_ops is introduced.
   * Pass frequency table entry structure to function calculate_m_and_n.
   * Remove desc from qcom_cc_register_rcg_dfs and instead pass array of
     clk_rcg2.
   * Add a dfs_enable flag to identify if dfs mode is enabled.

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.

Taniya Das (2):
  clk: qcom: Add support for RCG to register for DFS
  clk: qcom: gcc: Register QUPv3 RCGs for DFS on SDM845

 drivers/clk/qcom/clk-rcg.h    |   5 +
 drivers/clk/qcom/clk-rcg2.c   | 214 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/common.c     |  14 +--
 drivers/clk/qcom/common.h     |  16 +---
 drivers/clk/qcom/gcc-sdm845.c |  27 +++++-
 5 files changed, 250 insertions(+), 26 deletions(-)

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

[PATCH v2 1/2] clk: qcom: Add support for RCG to register for DFS

[Taniya Das]

Dynamic Frequency switch is a feature of clock controller by which request
from peripherals allows automatic switching frequency of input clock.
There are various performance levels associated to a root clock generator.
Register the root clock generators(RCG) to switch to use the dfs clock ops
in the cases where DFS is enabled.
The DFS clock ops would at runtime read the clock perf level registers to
identify the frequencies supported and update the frequency table
accordingly.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/clk-rcg.h  |   5 ++
 drivers/clk/qcom/clk-rcg2.c | 214 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/common.c   |  14 +--
 drivers/clk/qcom/common.h   |  16 +---
 4 files changed, 224 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index b209a2f..05e3584 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -5,6 +5,7 @@
 #define __QCOM_CLK_RCG_H__

 #include <linux/clk-provider.h>
+#include <linux/platform_device.h>
 #include "clk-regmap.h"

 struct freq_tbl {
@@ -160,5 +161,9 @@ 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_rcg2_dfs_ops;
+
+extern int qcom_cc_register_rcg_dfs(struct platform_device *pdev,
+			     struct clk_rcg2 **rcgs, int num_clks);

 #endif
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 52208d4..25b0560 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -12,6 +12,7 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/math64.h>
+#include <linux/slab.h>

 #include <asm/div64.h>

@@ -40,6 +41,14 @@
 #define N_REG			0xc
 #define D_REG			0x10

+/* Dynamic Frequency Scaling */
+#define MAX_PERF_LEVEL		16
+#define SE_CMD_DFSR_OFFSET	0x14
+#define SE_CMD_DFS_EN		BIT(0)
+#define SE_PERF_DFSR(level)	(0x1c + 0x4 * (level))
+#define SE_PERF_M_DFSR(level)	(0x5c + 0x4 * (level))
+#define SE_PERF_N_DFSR(level)	(0x9c + 0x4 * (level))
+
 enum freq_policy {
 	FLOOR,
 	CEIL,
@@ -929,3 +938,208 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
 	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
 };
 EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
+
+/* Common APIs to be used for DFS based RCGR */
+static int clk_rcg2_calculate_m_and_n(struct clk_hw *hw, u32 *mode,
+		unsigned long *prate, int level, struct freq_tbl *f)
+{
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+	struct clk_hw *phw;
+	u32 val, mask, cfg, m_off, n_off, offset;
+	int i, ret, num_parents;
+
+	offset = SE_PERF_DFSR(level);
+	ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + offset, &cfg);
+	if (ret)
+		return ret;
+
+	mask = BIT(rcg->hid_width) - 1;
+	f->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;
+
+	num_parents = clk_hw_get_num_parents(hw);
+	for (i = 0; i < num_parents; i++) {
+		if (cfg == rcg->parent_map[i].cfg) {
+			f->src = rcg->parent_map[i].src;
+			phw = clk_hw_get_parent_by_index(&rcg->clkr.hw, i);
+			*prate = clk_hw_get_rate(phw);
+		}
+	}
+
+	if (!*mode)
+		return 0;
+
+	/* Calculate M & N values */
+	m_off = SE_PERF_M_DFSR(level);
+	n_off = SE_PERF_N_DFSR(level);
+
+	mask = BIT(rcg->mnd_width) - 1;
+	ret =  regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + m_off, &val);
+	if (ret) {
+		pr_err("Failed to read M offset register\n");
+		return ret;
+	}
+	val &= mask;
+	f->m  = val;
+
+	ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + n_off, &val);
+	if (ret) {
+		pr_err("Failed to read N offset register\n");
+		return ret;
+	}
+	/* val ~(N-M) */
+	val = ~val;
+	val &= mask;
+	val += f->m;
+	f->n = val;
+
+	return 0;
+}
+
+static int clk_rcg2_dfs_populate_freq_table(struct clk_rcg2 *rcg)
+{
+	struct freq_tbl *dfs_freq_tbl;
+	int i, j, ret = 0;
+	unsigned long calc_freq, prate = 0;
+	u32 mode = 0;
+
+	dfs_freq_tbl = kcalloc(MAX_PERF_LEVEL, sizeof(struct freq_tbl),
+				GFP_KERNEL);
+	if (!dfs_freq_tbl)
+		return -ENOMEM;
+
+	for (i = 0; i < MAX_PERF_LEVEL; i++) {
+		ret = clk_rcg2_calculate_m_and_n(&rcg->clkr.hw, &mode, &prate,
+						 i, &dfs_freq_tbl[i]);
+		if (ret) {
+			kfree(dfs_freq_tbl);
+			return ret;
+		}
+
+		calc_freq = calc_rate(prate, dfs_freq_tbl[i].m,
+					dfs_freq_tbl[i].n, mode,
+					dfs_freq_tbl[i].pre_div);
+
+		/* Check for duplicate frequencies to end table */
+		for (j = 0; j  < i; j++) {
+			if (dfs_freq_tbl[j].freq == calc_freq)
+				goto done;
+		}
+
+		dfs_freq_tbl[i].freq = calc_freq;
+	}
+done:
+	rcg->freq_tbl = dfs_freq_tbl;
+
+	return 0;
+}
+
+static int clk_rcg2_dfs_determine_rate(struct clk_hw *hw,
+				   struct clk_rate_request *req)
+{
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+	int ret;
+
+	if (rcg->freq_tbl == NULL) {
+		ret = clk_rcg2_dfs_populate_freq_table(rcg);
+		if (ret) {
+			pr_err("Failed to update DFS tables\n");
+			return ret;
+		}
+	}
+
+	return clk_rcg2_shared_ops.determine_rate(hw, req);
+}
+
+static int clk_rcg2_dfs_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long prate)
+{
+	return 0;
+}
+
+static unsigned long
+clk_rcg2_dfs_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	return 0;
+}
+
+const struct clk_ops clk_rcg2_dfs_ops = {
+	.is_enabled = clk_rcg2_is_enabled,
+	.get_parent = clk_rcg2_get_parent,
+	.recalc_rate = clk_rcg2_dfs_recalc_rate,
+	.determine_rate = clk_rcg2_dfs_determine_rate,
+	.set_rate = clk_rcg2_dfs_set_rate,
+};
+EXPORT_SYMBOL_GPL(clk_rcg2_dfs_ops);
+
+static int clk_rcg2_enable_dfs(struct clk_rcg2 *rcg, struct device *dev)
+{
+	struct regmap *regmap;
+	struct clk_init_data *init;
+	struct clk_rate_request req = { };
+	u32 val;
+	int ret;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap)
+		return -EINVAL;
+
+	init = kzalloc(sizeof(struct clk_init_data), GFP_KERNEL);
+	if (!init)
+		return -ENOMEM;
+
+	init->name = rcg->clkr.hw.init->name;
+	init->ops = &clk_rcg2_shared_ops;
+	init->flags = rcg->clkr.hw.init->flags;
+	init->parent_names = rcg->clkr.hw.init->parent_names;
+	init->num_parents = rcg->clkr.hw.init->num_parents;
+	rcg->clkr.hw.init = init;
+
+	ret = regmap_read(regmap, rcg->cmd_rcgr + SE_CMD_DFSR_OFFSET,
+			  &val);
+	if (ret) {
+		dev_err(dev, "Failed to read DFS enable register\n");
+		return -EINVAL;
+	}
+
+	if (!(val & SE_CMD_DFS_EN))
+		return 0;
+
+	init->ops = &clk_rcg2_dfs_ops;
+	rcg->clkr.hw.init = init;
+	rcg->freq_tbl = NULL;
+
+	/*
+	 * In case the clocks are registered earlier than determine the dfs
+	 * rates.
+	 */
+	if (__clk_lookup(rcg->clkr.hw.init->name))
+		return clk_rcg2_dfs_determine_rate(&rcg->clkr.hw, &req);
+
+	return 0;
+}
+
+int qcom_cc_register_rcg_dfs(struct platform_device *pdev,
+			    struct clk_rcg2 **rcgs, int num_clks)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < num_clks; i++) {
+		ret = clk_rcg2_enable_dfs(rcgs[i], &pdev->dev);
+		if (ret) {
+			const char *name = rcgs[i]->clkr.hw.init->name;
+
+			dev_err(&pdev->dev,
+				"%s DFS frequencies update failed\n", name);
+			break;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_cc_register_rcg_dfs);
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 39ce64c..958d27c 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -1,15 +1,5 @@
-/*
- * Copyright (c) 2013-2014, 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
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved. */

 #include <linux/export.h>
 #include <linux/module.h>
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 00196ee..52d2dce 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -1,15 +1,6 @@
-/*
- * Copyright (c) 2014, 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
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2014, 2018, The Linux Foundation. All rights reserved. */
+
 #ifndef __QCOM_CLK_COMMON_H__
 #define __QCOM_CLK_COMMON_H__

@@ -68,5 +59,4 @@ extern int qcom_cc_really_probe(struct platform_device *pdev,
 				struct regmap *regmap);
 extern int qcom_cc_probe(struct platform_device *pdev,
 			 const struct qcom_cc_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.

Re: [PATCH v2 1/2] clk: qcom: Add support for RCG to register for DFS

[Stephen Boyd]

Quoting Taniya Das (2018-06-28 04:47:30)
> Dynamic Frequency switch is a feature of clock controller by which request
> from peripherals allows automatic switching frequency of input clock.
> There are various performance levels associated to a root clock generator.
> Register the root clock generators(RCG) to switch to use the dfs clock ops
> in the cases where DFS is enabled.
> The DFS clock ops would at runtime read the clock perf level registers to
> identify the frequencies supported and update the frequency table
> accordingly.

But nobody is really using the frequency table except for
clk_round_rate()? Can you add some comments into the commit text and the
code indicating how it works in hardware? I think the way it works is by
having DFS enabled in the clock controller, and then another register in
the QUP register space controls the index that the clk configures itself
for. But I'm not clear on if the RCG registers are updated when DFS
frequency is set, or if anything is visible from the clk controller
hardware besides being in DFS mode and the frequency plan.

> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 52208d4..25b0560 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -929,3 +938,208 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
>         .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
>  };
>  EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
> +
> +/* Common APIs to be used for DFS based RCGR */
> +static int clk_rcg2_calculate_m_and_n(struct clk_hw *hw, u32 *mode,
> +               unsigned long *prate, int level, struct freq_tbl *f)
> +{
> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +       struct clk_hw *phw;

Just 'p' for parent is good.

> +       u32 val, mask, cfg, m_off, n_off, offset;
> +       int i, ret, num_parents;
> +
> +       offset = SE_PERF_DFSR(level);
> +       ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + offset, &cfg);
> +       if (ret)
> +               return ret;
> +
> +       mask = BIT(rcg->hid_width) - 1;
> +       f->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;
> +
> +       num_parents = clk_hw_get_num_parents(hw);
> +       for (i = 0; i < num_parents; i++) {
> +               if (cfg == rcg->parent_map[i].cfg) {
> +                       f->src = rcg->parent_map[i].src;
> +                       phw = clk_hw_get_parent_by_index(&rcg->clkr.hw, i);
> +                       *prate = clk_hw_get_rate(phw);
> +               }
> +       }
> +
> +       if (!*mode)
> +               return 0;
> +
> +       /* Calculate M & N values */
> +       m_off = SE_PERF_M_DFSR(level);
> +       n_off = SE_PERF_N_DFSR(level);
> +
> +       mask = BIT(rcg->mnd_width) - 1;
> +       ret =  regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + m_off, &val);

Why weird space before regmap_read?

> +       if (ret) {
> +               pr_err("Failed to read M offset register\n");
> +               return ret;
> +       }
> +       val &= mask;
> +       f->m  = val;
> +
> +       ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + n_off, &val);
> +       if (ret) {
> +               pr_err("Failed to read N offset register\n");
> +               return ret;
> +       }
> +       /* val ~(N-M) */
> +       val = ~val;
> +       val &= mask;
> +       val += f->m;
> +       f->n = val;
> +
> +       return 0;
> +}
> +
> +static int clk_rcg2_dfs_populate_freq_table(struct clk_rcg2 *rcg)
> +{
> +       struct freq_tbl *dfs_freq_tbl;
> +       int i, j, ret = 0;
> +       unsigned long calc_freq, prate = 0;
> +       u32 mode = 0;
> +
> +       dfs_freq_tbl = kcalloc(MAX_PERF_LEVEL, sizeof(struct freq_tbl),

sizeof(*dfs_freq_tbl) or just call it freq_tbl. We know it's dfs
already.

> +                               GFP_KERNEL);
> +       if (!dfs_freq_tbl)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < MAX_PERF_LEVEL; i++) {
> +               ret = clk_rcg2_calculate_m_and_n(&rcg->clkr.hw, &mode, &prate,
> +                                                i, &dfs_freq_tbl[i]);

Why can't this function return the frequency? Or 0 if it failed for some
reason?

> +               if (ret) {
> +                       kfree(dfs_freq_tbl);
> +                       return ret;
> +               }
> +
> +               calc_freq = calc_rate(prate, dfs_freq_tbl[i].m,
> +                                       dfs_freq_tbl[i].n, mode,
> +                                       dfs_freq_tbl[i].pre_div);

Because this is not so good looking.

> +               /* Check for duplicate frequencies to end table */
> +               for (j = 0; j  < i; j++) {

Duplicate space after j here should be cleaned up.

> +                       if (dfs_freq_tbl[j].freq == calc_freq)

Is it sometimes a duplicate frequency that came earlier in the table?
Why isn't this just a check for the same frequency in succession while
going through the table? Or even a frequency that's lower than the
previous one?

> +                               goto done;
> +               }
> +
> +               dfs_freq_tbl[i].freq = calc_freq;
> +       }
> +done:
> +       rcg->freq_tbl = dfs_freq_tbl;
> +
> +       return 0;
> +}
> +
> +static int clk_rcg2_dfs_determine_rate(struct clk_hw *hw,
> +                                  struct clk_rate_request *req)
> +{
> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +       int ret;
> +
> +       if (rcg->freq_tbl == NULL) {

if (!rcg->freq_tbl) is preferred style.

> +               ret = clk_rcg2_dfs_populate_freq_table(rcg);
> +               if (ret) {
> +                       pr_err("Failed to update DFS tables\n");

Also print the clk name? Who knows which one this happens for otherwise.

> +                       return ret;
> +               }
> +       }
> +
> +       return clk_rcg2_shared_ops.determine_rate(hw, req);
> +}
> +
> +static int clk_rcg2_dfs_set_rate(struct clk_hw *hw, unsigned long rate,

We can't set the rate? 

> +                                   unsigned long prate)
> +{
> +       return 0;
> +}
> +
> +static unsigned long
> +clk_rcg2_dfs_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)

Huh? How will we know what frequency the clk is running at? We can't?

Why are these even implemented then? Just implement determine_rate() op
so that clk_round_rate() can be called in a loop to figure out the
frequency plan index? Or if the registers are actually updated when QUP
asks for an index, then recalc_rate() should be implemented and the
nocache flag should be added to the clk so we know to recalculate each
time clk_get_rate() is called.

> +{
> +       return 0;
> +}
> +
> +const struct clk_ops clk_rcg2_dfs_ops = {

static?

> +       .is_enabled = clk_rcg2_is_enabled,
> +       .get_parent = clk_rcg2_get_parent,
> +       .recalc_rate = clk_rcg2_dfs_recalc_rate,
> +       .determine_rate = clk_rcg2_dfs_determine_rate,
> +       .set_rate = clk_rcg2_dfs_set_rate,
> +};
> +EXPORT_SYMBOL_GPL(clk_rcg2_dfs_ops);
> +
> +static int clk_rcg2_enable_dfs(struct clk_rcg2 *rcg, struct device *dev)
> +{
> +       struct regmap *regmap;
> +       struct clk_init_data *init;
> +       struct clk_rate_request req = { };
> +       u32 val;
> +       int ret;
> +
> +       regmap = dev_get_regmap(dev, NULL);
> +       if (!regmap)
> +               return -EINVAL;
> +
> +       init = kzalloc(sizeof(struct clk_init_data), GFP_KERNEL);

Use sizeof(*init) so type doesn't matter.

> +       if (!init)
> +               return -ENOMEM;
> +
> +       init->name = rcg->clkr.hw.init->name;
> +       init->ops = &clk_rcg2_shared_ops;

These should already be done statically.

> +       init->flags = rcg->clkr.hw.init->flags;
> +       init->parent_names = rcg->clkr.hw.init->parent_names;
> +       init->num_parents = rcg->clkr.hw.init->num_parents;
> +       rcg->clkr.hw.init = init;

In fact, the whole init structure should just be whatever is there
already.

> +
> +       ret = regmap_read(regmap, rcg->cmd_rcgr + SE_CMD_DFSR_OFFSET,
> +                         &val);
> +       if (ret) {
> +               dev_err(dev, "Failed to read DFS enable register\n");

For which clk? And do we even care? The regmap_read most likely never
fails.

> +               return -EINVAL;
> +       }
> +
> +       if (!(val & SE_CMD_DFS_EN))
> +               return 0;
> +
> +       init->ops = &clk_rcg2_dfs_ops;
> +       rcg->clkr.hw.init = init;

The clks were already registered. I don't know how this works.

> +       rcg->freq_tbl = NULL;
> +
> +       /*
> +        * In case the clocks are registered earlier than determine the dfs
> +        * rates.
> +        */
> +       if (__clk_lookup(rcg->clkr.hw.init->name))
> +               return clk_rcg2_dfs_determine_rate(&rcg->clkr.hw, &req);

Remove this. Do the DFS enabled probing before registering the clks.

> +
> +       return 0;
> +}
> +
> +int qcom_cc_register_rcg_dfs(struct platform_device *pdev,

Please don't pass the device. Pass the regmap.

> +                           struct clk_rcg2 **rcgs, int num_clks)
> +{
> +       int i, ret = 0;
> +
> +       for (i = 0; i < num_clks; i++) {
> +               ret = clk_rcg2_enable_dfs(rcgs[i], &pdev->dev);
> +               if (ret) {
> +                       const char *name = rcgs[i]->clkr.hw.init->name;
> +
> +                       dev_err(&pdev->dev,
> +                               "%s DFS frequencies update failed\n", name);
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_cc_register_rcg_dfs);

> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 39ce64c..958d27c 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -1,15 +1,5 @@
> -/*
> - * Copyright (c) 2013-2014, 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
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved. */
> 
>  #include <linux/export.h>
>  #include <linux/module.h>
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 00196ee..52d2dce 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -1,15 +1,6 @@
> -/*
> - * Copyright (c) 2014, 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
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2014, 2018, The Linux Foundation. All rights reserved. */
> +

Can you split these last two file hunks into another patch to update
common for SPDX? I can apply that without anything else.

>  #ifndef __QCOM_CLK_COMMON_H__
>  #define __QCOM_CLK_COMMON_H__
> 
> @@ -68,5 +59,4 @@ extern int qcom_cc_really_probe(struct platform_device *pdev,
>                                 struct regmap *regmap);
>  extern int qcom_cc_probe(struct platform_device *pdev,
>                          const struct qcom_cc_desc *desc);
> -

No.

>  #endif
> --

Re: [PATCH v2 1/2] clk: qcom: Add support for RCG to register for DFS

[Taniya Das]

Hello Stephen,

Thanks for your review comments.

On 7/9/2018 12:34 PM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-06-28 04:47:30)
>> Dynamic Frequency switch is a feature of clock controller by which request
>> from peripherals allows automatic switching frequency of input clock.
>> There are various performance levels associated to a root clock generator.
>> Register the root clock generators(RCG) to switch to use the dfs clock ops
>> in the cases where DFS is enabled.
>> The DFS clock ops would at runtime read the clock perf level registers to
>> identify the frequencies supported and update the frequency table
>> accordingly.
> 
> But nobody is really using the frequency table except for
> clk_round_rate()? Can you add some comments into the commit text and the
> code indicating how it works in hardware? I think the way it works is by
> having DFS enabled in the clock controller, and then another register in
> the QUP register space controls the index that the clk configures itself
> for. But I'm not clear on if the RCG registers are updated when DFS
> frequency is set, or if anything is visible from the clk controller
> hardware besides being in DFS mode and the frequency plan.
> 
>>
Sure, added few more details in the next series.

>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
>> index 52208d4..25b0560 100644
>> --- a/drivers/clk/qcom/clk-rcg2.c
>> +++ b/drivers/clk/qcom/clk-rcg2.c
>> @@ -929,3 +938,208 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
>>          .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
>>   };
>>   EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
>> +
>> +/* Common APIs to be used for DFS based RCGR */
>> +static int clk_rcg2_calculate_m_and_n(struct clk_hw *hw, u32 *mode,
>> +               unsigned long *prate, int level, struct freq_tbl *f)
>> +{
>> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
>> +       struct clk_hw *phw;
> 
> Just 'p' for parent is good.
> 

Taken care in the next patch.

>> +       u32 val, mask, cfg, m_off, n_off, offset;
>> +       int i, ret, num_parents;
>> +
>> +       offset = SE_PERF_DFSR(level);
>> +       ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + offset, &cfg);
>> +       if (ret)
>> +               return ret;
>> +
>> +       mask = BIT(rcg->hid_width) - 1;
>> +       f->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;
>> +
>> +       num_parents = clk_hw_get_num_parents(hw);
>> +       for (i = 0; i < num_parents; i++) {
>> +               if (cfg == rcg->parent_map[i].cfg) {
>> +                       f->src = rcg->parent_map[i].src;
>> +                       phw = clk_hw_get_parent_by_index(&rcg->clkr.hw, i);
>> +                       *prate = clk_hw_get_rate(phw);
>> +               }
>> +       }
>> +
>> +       if (!*mode)
>> +               return 0;
>> +
>> +       /* Calculate M & N values */
>> +       m_off = SE_PERF_M_DFSR(level);
>> +       n_off = SE_PERF_N_DFSR(level);
>> +
>> +       mask = BIT(rcg->mnd_width) - 1;
>> +       ret =  regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + m_off, &val);
> 
> Why weird space before regmap_read?
> 

removed.

>> +       if (ret) {
>> +               pr_err("Failed to read M offset register\n");
>> +               return ret;
>> +       }
>> +       val &= mask;
>> +       f->m  = val;
>> +
>> +       ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + n_off, &val);
>> +       if (ret) {
>> +               pr_err("Failed to read N offset register\n");
>> +               return ret;
>> +       }
>> +       /* val ~(N-M) */
>> +       val = ~val;
>> +       val &= mask;
>> +       val += f->m;
>> +       f->n = val;
>> +
>> +       return 0;
>> +}
>> +
>> +static int clk_rcg2_dfs_populate_freq_table(struct clk_rcg2 *rcg)
>> +{
>> +       struct freq_tbl *dfs_freq_tbl;
>> +       int i, j, ret = 0;
>> +       unsigned long calc_freq, prate = 0;
>> +       u32 mode = 0;
>> +
>> +       dfs_freq_tbl = kcalloc(MAX_PERF_LEVEL, sizeof(struct freq_tbl),
> 
> sizeof(*dfs_freq_tbl) or just call it freq_tbl. We know it's dfs
> already.
> 

Fixed in the next patch.

>> +                               GFP_KERNEL);
>> +       if (!dfs_freq_tbl)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < MAX_PERF_LEVEL; i++) {
>> +               ret = clk_rcg2_calculate_m_and_n(&rcg->clkr.hw, &mode, &prate,
>> +                                                i, &dfs_freq_tbl[i]);
> 
> Why can't this function return the frequency? Or 0 if it failed for some
> reason?
> 

Sure, have taken care in the next patch.

>> +               if (ret) {
>> +                       kfree(dfs_freq_tbl);
>> +                       return ret;
>> +               }
>> +
>> +               calc_freq = calc_rate(prate, dfs_freq_tbl[i].m,
>> +                                       dfs_freq_tbl[i].n, mode,
>> +                                       dfs_freq_tbl[i].pre_div);
> 
> Because this is not so good looking.
> 
>> +               /* Check for duplicate frequencies to end table */
>> +               for (j = 0; j  < i; j++) {
> 
> Duplicate space after j here should be cleaned up.
> 
>> +                       if (dfs_freq_tbl[j].freq == calc_freq)
> 
> Is it sometimes a duplicate frequency that came earlier in the table?
> Why isn't this just a check for the same frequency in succession while
> going through the table? Or even a frequency that's lower than the
> previous one?
> 

I was trimming the table based on the last frequencies, say we have 
multiple 19.2MHz calculated at the end, so I was trimming the table, but 
I don't think it is really required. So I have removed this logic.
>> +                               goto done;
>> +               }
>> +
>> +               dfs_freq_tbl[i].freq = calc_freq;
>> +       }
>> +done:
>> +       rcg->freq_tbl = dfs_freq_tbl;
>> +
>> +       return 0;
>> +}
>> +
>> +static int clk_rcg2_dfs_determine_rate(struct clk_hw *hw,
>> +                                  struct clk_rate_request *req)
>> +{
>> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
>> +       int ret;
>> +
>> +       if (rcg->freq_tbl == NULL) {
> 
> if (!rcg->freq_tbl) is preferred style.
> 
>> +               ret = clk_rcg2_dfs_populate_freq_table(rcg);
>> +               if (ret) {
>> +                       pr_err("Failed to update DFS tables\n");
> 
> Also print the clk name? Who knows which one this happens for otherwise.
> 

Taken care in the next patch.

>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       return clk_rcg2_shared_ops.determine_rate(hw, req);
>> +}
>> +
>> +static int clk_rcg2_dfs_set_rate(struct clk_hw *hw, unsigned long rate,
> 
> We can't set the rate?
>

No, if it is DFS mode, SW would not be allowed to update the RCG.
I have removed the set_rate in the next series.

>> +                                   unsigned long prate)
>> +{
>> +       return 0;
>> +}
>> +
>> +static unsigned long
>> +clk_rcg2_dfs_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> 
> Huh? How will we know what frequency the clk is running at? We can't?
> 
> Why are these even implemented then? Just implement determine_rate() op
> so that clk_round_rate() can be called in a loop to figure out the
> frequency plan index? Or if the registers are actually updated when QUP
> asks for an index, then recalc_rate() should be implemented and the
> nocache flag should be added to the clk so we know to recalculate each
> time clk_get_rate() is called.
> 

No, we can't read any registers to know the frequency at which the clock
is running. I have removed this too in the next series.
>> +{
>> +       return 0;
>> +}
>> +
>> +const struct clk_ops clk_rcg2_dfs_ops = {
> 
> static?
>
Done.

>> +       .is_enabled = clk_rcg2_is_enabled,
>> +       .get_parent = clk_rcg2_get_parent,
>> +       .recalc_rate = clk_rcg2_dfs_recalc_rate,
>> +       .determine_rate = clk_rcg2_dfs_determine_rate,
>> +       .set_rate = clk_rcg2_dfs_set_rate,
>> +};
>> +EXPORT_SYMBOL_GPL(clk_rcg2_dfs_ops);
>> +
>> +static int clk_rcg2_enable_dfs(struct clk_rcg2 *rcg, struct device *dev)
>> +{
>> +       struct regmap *regmap;
>> +       struct clk_init_data *init;
>> +       struct clk_rate_request req = { };
>> +       u32 val;
>> +       int ret;
>> +
>> +       regmap = dev_get_regmap(dev, NULL);
>> +       if (!regmap)
>> +               return -EINVAL;
>> +
>> +       init = kzalloc(sizeof(struct clk_init_data), GFP_KERNEL);
> 
> Use sizeof(*init) so type doesn't matter.
> 
Done.

>> +       if (!init)
>> +               return -ENOMEM;
>> +
>> +       init->name = rcg->clkr.hw.init->name;
>> +       init->ops = &clk_rcg2_shared_ops;
> 
> These should already be done statically.
> 
>> +       init->flags = rcg->clkr.hw.init->flags;
>> +       init->parent_names = rcg->clkr.hw.init->parent_names;
>> +       init->num_parents = rcg->clkr.hw.init->num_parents;
>> +       rcg->clkr.hw.init = init;
> 
> In fact, the whole init structure should just be whatever is there
> already.
>
I would override it now, only if DFS ops is required.

>> +
>> +       ret = regmap_read(regmap, rcg->cmd_rcgr + SE_CMD_DFSR_OFFSET,
>> +                         &val);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to read DFS enable register\n");
> 
> For which clk? And do we even care? The regmap_read most likely never
> fails.
> 
Removed the print.

>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!(val & SE_CMD_DFS_EN))
>> +               return 0;
>> +
>> +       init->ops = &clk_rcg2_dfs_ops;
>> +       rcg->clkr.hw.init = init;
> 
> The clks were already registered. I don't know how this works.
> 
Yes, it worked and it updated the ops.

>> +       rcg->freq_tbl = NULL;
>> +
>> +       /*
>> +        * In case the clocks are registered earlier than determine the dfs
>> +        * rates.
>> +        */
>> +       if (__clk_lookup(rcg->clkr.hw.init->name))
>> +               return clk_rcg2_dfs_determine_rate(&rcg->clkr.hw, &req);
> 
> Remove this. Do the DFS enabled probing before registering the clks.
> 
Removed.
>> +
>> +       return 0;
>> +}
>> +
>> +int qcom_cc_register_rcg_dfs(struct platform_device *pdev,
> 
> Please don't pass the device. Pass the regmap.
> 
>> +                           struct clk_rcg2 **rcgs, int num_clks)
>> +{
>> +       int i, ret = 0;
>> +
>> +       for (i = 0; i < num_clks; i++) {
>> +               ret = clk_rcg2_enable_dfs(rcgs[i], &pdev->dev);
>> +               if (ret) {
>> +                       const char *name = rcgs[i]->clkr.hw.init->name;
>> +
>> +                       dev_err(&pdev->dev,
>> +                               "%s DFS frequencies update failed\n", name);
>> +                       break;
>> +               }
>> +       }
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_cc_register_rcg_dfs);
> 
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index 39ce64c..958d27c 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -1,15 +1,5 @@
>> -/*
>> - * Copyright (c) 2013-2014, 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
>> - * may be copied, distributed, and modified under those terms.
>> - *
>> - * This program is distributed in the hope that it will be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> - * GNU General Public License for more details.
>> - */
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved. */
>>
>>   #include <linux/export.h>
>>   #include <linux/module.h>
>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>> index 00196ee..52d2dce 100644
>> --- a/drivers/clk/qcom/common.h
>> +++ b/drivers/clk/qcom/common.h
>> @@ -1,15 +1,6 @@
>> -/*
>> - * Copyright (c) 2014, 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
>> - * may be copied, distributed, and modified under those terms.
>> - *
>> - * This program is distributed in the hope that it will be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> - * GNU General Public License for more details.
>> - */
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2014, 2018, The Linux Foundation. All rights reserved. */
>> +
> 
> Can you split these last two file hunks into another patch to update
> common for SPDX? I can apply that without anything else.
> 

Removed these patches.

>>   #ifndef __QCOM_CLK_COMMON_H__
>>   #define __QCOM_CLK_COMMON_H__
>>
>> @@ -68,5 +59,4 @@ extern int qcom_cc_really_probe(struct platform_device *pdev,
>>                                  struct regmap *regmap);
>>   extern int qcom_cc_probe(struct platform_device *pdev,
>>                           const struct qcom_cc_desc *desc);
>> -
> 
> No.
> 
>>   #endif
>> --

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

--

[PATCH v2 2/2] clk: qcom: gcc: Register QUPv3 RCGs for DFS on SDM845

[Taniya Das]

QUPv3 clocks support DFS and thus register the RCGs which require support
for the same.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/gcc-sdm845.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
index e78e6f5..034c762 100644
--- a/drivers/clk/qcom/gcc-sdm845.c
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -3421,9 +3421,29 @@ enum {
 };
 MODULE_DEVICE_TABLE(of, gcc_sdm845_match_table);

+static struct clk_rcg2 *gcc_dfs_clocks[] = {
+	&gcc_qupv3_wrap0_s0_clk_src,
+	&gcc_qupv3_wrap0_s1_clk_src,
+	&gcc_qupv3_wrap0_s2_clk_src,
+	&gcc_qupv3_wrap0_s3_clk_src,
+	&gcc_qupv3_wrap0_s4_clk_src,
+	&gcc_qupv3_wrap0_s5_clk_src,
+	&gcc_qupv3_wrap0_s6_clk_src,
+	&gcc_qupv3_wrap0_s7_clk_src,
+	&gcc_qupv3_wrap1_s0_clk_src,
+	&gcc_qupv3_wrap1_s1_clk_src,
+	&gcc_qupv3_wrap1_s2_clk_src,
+	&gcc_qupv3_wrap1_s3_clk_src,
+	&gcc_qupv3_wrap1_s4_clk_src,
+	&gcc_qupv3_wrap1_s5_clk_src,
+	&gcc_qupv3_wrap1_s6_clk_src,
+	&gcc_qupv3_wrap1_s7_clk_src,
+};
+
 static int gcc_sdm845_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
+	int ret;

 	regmap = qcom_cc_map(pdev, &gcc_sdm845_desc);
 	if (IS_ERR(regmap))
@@ -3437,7 +3457,12 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
 	regmap_update_bits(regmap, 0x48190, BIT(0), 0x1);
 	regmap_update_bits(regmap, 0x52004, BIT(22), 0x1);

-	return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
+	if (ret)
+		return ret;
+
+	return qcom_cc_register_rcg_dfs(pdev, gcc_dfs_clocks,
+					ARRAY_SIZE(gcc_dfs_clocks));
 }

 static struct platform_driver gcc_sdm845_driver = {
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v2 2/2] clk: qcom: gcc: Register QUPv3 RCGs for DFS on SDM845

[Stephen Boyd]

Quoting Taniya Das (2018-06-28 04:47:31)
> @@ -3437,7 +3457,12 @@ static int gcc_sdm845_probe(struct platform_device *pdev)
>         regmap_update_bits(regmap, 0x48190, BIT(0), 0x1);
>         regmap_update_bits(regmap, 0x52004, BIT(22), 0x1);
> 
> -       return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
> +       ret = qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
> +       if (ret)
> +               return ret;
> +
> +       return qcom_cc_register_rcg_dfs(pdev, gcc_dfs_clocks,
> +                                       ARRAY_SIZE(gcc_dfs_clocks));

This looks backwards. We shouldn't expose the clks to drivers and then
make their functionality work by registering dfs clks. The order should
be swapped.