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@xxxxxxxxxxxxxx>
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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
