Re: [PATCH v1] clk: qcom: Add support for RCG to register for DFS

From: Stephen Boyd
Date: Tue Jun 12 2018 - 03:17:40 EST

Next message: Geert Uytterhoeven: "[PATCH] autofs: Fix typo s/thenew new/the new/ in AUTOFS4_FS description"
Previous message: Boris Brezillon: "Re: [PATCH 03/11] spi: Add a driver for the Freescale/NXP QuadSPI controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Quoting Taniya Das (2018-05-25 03:45:50)
> 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.

This sentence doesn't parse. Please rewrite it into multiple sentences
and explain what DFS is and why it's used and why we care to merge this
patch.

>
> Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx>

> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> index b209a2f..b5e5424 100644
> --- a/drivers/clk/qcom/clk-rcg.h
> +++ b/drivers/clk/qcom/clk-rcg.h
> @@ -138,6 +138,7 @@ struct clk_dyn_rcg {
> * @parent_map: map from software's parent index to hardware's src_sel field
> * @freq_tbl: frequency table
> * @clkr: regmap clock handle
> + * @dfs_enable: corresponds to dfs mode enable

Please call this 'dfs_enabled'.

> *

Also drop this extra newline here since you're in the area.

> */
> struct clk_rcg2 {
> @@ -160,5 +162,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 clk_rcg2_enable_dfs(struct clk_rcg2 *clk, struct device *dev);
> +extern int clk_rcg2_dfs_determine_rate_lazy(struct clk_rcg2 *clk);

This doesn't need to be exported?

>
> #endif
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 52208d4..9112891 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -10,8 +10,10 @@
> #include <linux/export.h>
> #include <linux/clk-provider.h>
> #include <linux/delay.h>
> +#include <linux/device.h>

Looks odd. Why not pass regmap instead?

> @@ -929,3 +939,265 @@ 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 */
> +struct dfs_table {
> + u8 src_index;
> + unsigned long prate;
> +};
> +
> +static struct dfs_table *dfs_entry;

So all DFS are the same? Doesn't look right to have a static global for
this.

> +
> +static int clk_index_pre_div_and_mode(struct clk_hw *hw, u32 offset,

Please prefix with clk_rcg2_ on all these things.

> + u32 *mode, u8 *pre_div)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> + int i, num_parents, ret;
> + u32 cfg, mask;
> +
> + num_parents = clk_hw_get_num_parents(hw);
> +
> + ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + offset, &cfg);
> + 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;
> +
> + 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,

Doesn't M and N always follow exactly same place? Why does this need to
be passed in here, just hardcode it?

> + struct freq_tbl *f)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> + u32 val, mask;
> + int ret;
> +
> + /* 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");
> + return ret;
> + }
> +
> + val &= mask;
> + f->m = val;

Weird space here.

> +
> + ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + n_offset, &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;
> +}
> +
> +int clk_rcg2_dfs_determine_rate_lazy(struct clk_rcg2 *rcg)

static?

> +{
> + struct freq_tbl *dfs_freq_tbl;
> + int i, j, index, ret = 0;
> + unsigned long calc_freq, prate;
> + u32 mode = 0;
> +
> + if (rcg->dfs_enable) {
> + pr_debug("DFS tables already populated\n");
> + return 0;
> + }
> +
> + dfs_freq_tbl = kcalloc(MAX_PERF_LEVEL, sizeof(struct freq_tbl),

sizeof(*dfs_freq_tbl)

> + GFP_KERNEL);
> + if (!dfs_freq_tbl)
> + return -ENOMEM;
> +
> + /* Populate the Perf Level frequencies */
> + for (i = 0; i < MAX_PERF_LEVEL; i++) {
> + /* Get parent index and mode */
> + index = clk_index_pre_div_and_mode(&rcg->clkr.hw,
> + SE_PERF_DFSR(i), &mode,
> + &dfs_freq_tbl[i].pre_div);
> + if (index < 0) {
> + pr_err("Failed to get parent index & mode %d\n", index);
> + kzfree(dfs_freq_tbl);
> + return index;
> + }
> +
> + /* Fill parent src */
> + dfs_freq_tbl[i].src = rcg->parent_map[index].src;
> + prate = dfs_entry[index].prate;
> +
> + if (mode) {
> + ret = calculate_m_and_n(&rcg->clkr.hw,
> + SE_PERF_M_DFSR(i),
> + SE_PERF_N_DFSR(i),
> + &dfs_freq_tbl[i]);

Please combine the prediv, mode, m, and n calculating code into one
function.

> + if (ret)
> + goto err;
> + } else {
> + dfs_freq_tbl[i].m = 0;
> + dfs_freq_tbl[i].n = 0;

Is this even necessary? The whole table is kcalloc() so it's already
zero.

> + }
> +
> + /* calculate the final frequency */

Please remove worthless comments.

> + 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 */

Sure, but why? Does it mean end of 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;
> +err:

This label should go away and the goto site should just return ret.

> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(clk_rcg2_dfs_determine_rate_lazy);
> +
> +static int _freq_tbl_determine_dfs_rate(struct clk_hw *hw)

Call this something like clk_rcg2_dfs_populate_freqtable()? Reading the
function though it looks like it's populating some parent frequencies
and src to software index table? I don't think we want to be populating
that at runtime. That parent to software index should be static in
hardware so I'm sort of lost on that.

> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> + struct clk_hw *phw;
> + int i, num_parents;
> +
> + num_parents = clk_hw_get_num_parents(hw);
> +
> + dfs_entry = kcalloc(num_parents, sizeof(struct dfs_table), GFP_KERNEL);

sizeof(*dfs_entry)

> + if (!dfs_entry)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_parents; i++) {
> + dfs_entry[i].src_index = rcg->parent_map[i].src;
> + if (rcg->parent_map[i].cfg == 7)

Why is 7 magical and special? Means grounded or something?

> + break;
> + phw = clk_hw_get_parent_by_index(&rcg->clkr.hw, i);
> + if (!phw) {
> + kzfree(dfs_entry);

Why kzfree()?

> + return -EINVAL;
> + }
> + dfs_entry[i].prate = clk_hw_get_rate(phw);
> + }
> +
> + 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->dfs_enable) {
> + ret = _freq_tbl_determine_dfs_rate(hw);
> + if (ret) {
> + pr_err("Failed to setup DFS frequency table\n");
> + return ret;
> + }
> +
> + ret = clk_rcg2_dfs_determine_rate_lazy(rcg);

Not sure we need to say 'lazy' in the name.

> + 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_parent(struct clk_hw *hw, u8 index)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +
> + /* DFS hardware takes care of setting the parent */
> + if (rcg->dfs_enable)

Why can't we read the hardware to see if DFS is there or not, and then
replace the clk_ops at boot time with the either the shared_ops or the
dfs "detecting" ones? That would make things a lot nicer so that we
don't have to look at these functions that swizzle between one or the
other every time they get called.

Due to the const on the clk_init pointer in clk_hw, you'll need to split
out the init structure into a named structure that you can modify at
runtime. Assign the default value to be the backup ops (shared?), and
then replace those ops with the dfs ops if the bit turns out to be set.
Then the dfs ops can be simple, except for the determine_rate callback.
And then dfs_enabled can be removed and we can set the freq_tbl pointer
to NULL on that clk_ops init swizzle and then populate the frequency
table if it's NULL in the determine rate op.

> + return 0;
> +
> + return clk_rcg2_shared_ops.set_parent(hw, index);
> +}
> +
> +static int clk_rcg2_dfs_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long prate)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +
> + if (rcg->dfs_enable)
> + return 0;
> +
> + return clk_rcg2_shared_ops.set_rate(hw, rate, prate);
> +}
> +
> +static int clk_rcg2_dfs_set_rate_and_parent(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long prate, u8 index)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +
> + if (rcg->dfs_enable)
> + return 0;
> +
> + return clk_rcg2_shared_ops.set_rate_and_parent(hw, rate, prate, index);
> +}
> +
> +const struct clk_ops clk_rcg2_dfs_ops = {
> + .is_enabled = clk_rcg2_is_enabled,
> + .get_parent = clk_rcg2_get_parent,
> + .set_parent = clk_rcg2_dfs_set_parent,
> + .recalc_rate = clk_rcg2_recalc_rate,

Does recalc_rate even work when DFS is enabled? How do we know what the
rate actually is if DFS is enabled and a driver calls clk_set_rate()?
Does the hardware update the registers to indicate what frequency is
selected at the time?

> + .determine_rate = clk_rcg2_dfs_determine_rate,
> + .set_rate = clk_rcg2_dfs_set_rate,
> + .set_rate_and_parent = clk_rcg2_dfs_set_rate_and_parent,
> +};
> +EXPORT_SYMBOL_GPL(clk_rcg2_dfs_ops);
> +
> +int clk_rcg2_enable_dfs(struct clk_rcg2 *rcg, struct device *dev)
> +{
> + struct regmap *regmap;
> + struct clk_rate_request req = { };
> + u32 val;
> + int ret;
> +
> + regmap = dev_get_regmap(dev, NULL);
> + if (!regmap)

Seems impossible! Of course, just pass the regmap and then it is 100%
impossible.

> + return -EINVAL;
> +
> + /* Check for DFS_EN */

Useless comment. Remove.

> + 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 ret;

Just return 0 please. It just happens to be 0 because it didn't fail
earlier, but 0 is more explicit about what's going on.

> +
> + ret = __clk_determine_rate(&rcg->clkr.hw, &req);
> + if (ret)
> + return ret;

The whole point of lazy evaluation is to not do this and only calculate
the rate when we know the parent frequency. Right now, that won't work
because 'req' is empty and put on the stack. I'm not sure why this whole
function exists right now.

> +
> + rcg->dfs_enable = true;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(clk_rcg2_enable_dfs);
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index b8064a3..0b656ed 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -288,4 +288,26 @@ 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,
> + struct clk_rcg2 **rcgs, int num_clks)
> +{
> + struct clk_rcg2 *rcg;
> + int i, ret = 0;
> +
> + for (i = 0; i < num_clks; i++) {
> + rcg = rcgs[i];
> + ret = clk_rcg2_enable_dfs(rcg, &pdev->dev);
> + if (ret) {
> + const char *name = (rcg->clkr.hw.init->name);

Useless parenthesis. Remove.

> +
> + dev_err(&pdev->dev,
> + "%s DFS frequencies update failed\n", name);
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_cc_register_rcg_dfs);

This whole thing should be in rcg.c, not common.c

> +
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 00196ee..71d1c27 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> #define PLL_LOCK_COUNT_MASK 0x3f
> @@ -69,4 +61,6 @@ 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,
> + struct clk_rcg2 **rcgs, int num_clks);

This should be exposed in clk-rcg.h instead of common.h

Next message: Geert Uytterhoeven: "[PATCH] autofs: Fix typo s/thenew new/the new/ in AUTOFS4_FS description"
Previous message: Boris Brezillon: "Re: [PATCH 03/11] spi: Add a driver for the Freescale/NXP QuadSPI controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]