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

[Stephen Boyd]

Quoting Taniya Das (2018-08-21 04:36:20)
> On 8/18/2018 11:31 PM, Taniya Das wrote:
> > Hello Stephen,
> > 
> > I will test these changes and get back.
> > 
> > On 8/18/2018 7:42 AM, Stephen Boyd wrote:
> >> Quoting Taniya Das (2018-08-10 18:53:54)
> >>> Â [v4]
> >>> ÂÂ * Add recalc_clk_ops to calculate the clock frequency reading the 
> >>> current
> >>> ÂÂÂÂ perf state, also add CLK_GET_RATE_NOCACHE flag.
> >>> ÂÂ * Cleanup 'goto' during mode check in 'clk_rcg2_calculate_freq'.
> >>> ÂÂ * cleanup return from function 'com_cc_register_rcg_dfs'.
> >>
> >> I want to squash this in. I have only compile tested it. Let me know
> >> what you think.
> >>
> >> ----8<---
> >> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> >> index e6300e05d5df..e5eca8a1abe4 100644
> >> --- a/drivers/clk/qcom/clk-rcg.h
> >> +++ b/drivers/clk/qcom/clk-rcg.h
> >> @@ -163,6 +163,15 @@ 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;
> >> +struct clk_rcg_dfs_data {
> >> +ÂÂÂ struct clk_rcg2 *rcg;
> >> +ÂÂÂ struct clk_init_data *init;
> >> +};
> >> +
> >> +#define DEFINE_RCG_DFS(r) \
> >> +ÂÂÂ { .rcg = &r##_src, .init = &r##_init }
> >> +
> >> Â extern int qcom_cc_register_rcg_dfs(struct regmap *regmap,
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct clk_rcg2 **rcgs, int num_clks);
> >> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const struct clk_rcg_dfs_data *rcgs,
> >> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t len);
> >> Â #endif
> >> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> >> index 55a5b58cbb15..bbe2a1916296 100644
> >> --- a/drivers/clk/qcom/clk-rcg2.c
> >> +++ b/drivers/clk/qcom/clk-rcg2.c
> >> @@ -1051,48 +1051,24 @@ static unsigned long
> >> Â clk_rcg2_dfs_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> >> Â {
> >> ÂÂÂÂÂ struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> >> -ÂÂÂ u32 cfg, hid_div, m = 0, n = 0, mode = 0, mask, level;
> >> -ÂÂÂ int num_parents, i;
> >> -ÂÂÂ unsigned long prate;
> >> -
> >> -ÂÂÂ regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr +
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SE_CMD_DFSR_OFFSET, &cfg);
> >> -ÂÂÂ level = (GENMASK(4, 1) & cfg) >> 1;
> >> -
> >> -ÂÂÂ regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr +
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SE_PERF_DFSR(level), &cfg);
> >> -ÂÂÂ if (rcg->mnd_width) {
> >> -ÂÂÂÂÂÂÂ mask = BIT(rcg->mnd_width) - 1;
> >> -ÂÂÂÂÂÂÂ regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr +
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SE_PERF_M_DFSR(level), &m);
> >> -ÂÂÂÂÂÂÂ m &= mask;
> >> -ÂÂÂÂÂÂÂ regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr +
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SE_PERF_N_DFSR(level), &n);
> >> -ÂÂÂÂÂÂÂ n =Â ~n;
> >> -ÂÂÂÂÂÂÂ n &= mask;
> >> -ÂÂÂÂÂÂÂ n += m;
> >> -ÂÂÂÂÂÂÂ mode = cfg & CFG_MODE_MASK;
> >> -ÂÂÂÂÂÂÂ mode >>= CFG_MODE_SHIFT;
> >> -ÂÂÂ }
> >> +ÂÂÂ int ret;
> >> +ÂÂÂ u32 level;
> >> -ÂÂÂ mask = BIT(rcg->hid_width) - 1;
> >> -ÂÂÂ hid_div = cfg >> CFG_SRC_DIV_SHIFT;
> >> -ÂÂÂ hid_div &= mask;
> >> -ÂÂÂ cfg &= CFG_SRC_SEL_MASK;
> >> -ÂÂÂ cfg >>= CFG_SRC_SEL_SHIFT;
> >> +ÂÂÂ regmap_read(rcg->clkr.regmap,
> >> +ÂÂÂÂÂÂÂÂÂÂÂ rcg->cmd_rcgr + SE_CMD_DFSR_OFFSET, &level);
> >> +ÂÂÂ level &= GENMASK(4, 1);
> >> +ÂÂÂ level >>= 1;
> >> -ÂÂÂ num_parents = clk_hw_get_num_parents(hw);
> >> -ÂÂÂ for (i = 0; i < num_parents; i++) {
> >> -ÂÂÂÂÂÂÂ if (cfg == rcg->parent_map[i].cfg) {
> >> -ÂÂÂÂÂÂÂÂÂÂÂ prate = clk_hw_get_rate(
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ clk_hw_get_parent_by_index(&rcg->clkr.hw, i));
> >> -ÂÂÂÂÂÂÂÂÂÂÂ if (parent_rate != prate)
> >> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ parent_rate = prate;
> >> +ÂÂÂ if (!rcg->freq_tbl) {
> >> +ÂÂÂÂÂÂÂ ret = clk_rcg2_dfs_populate_freq_table(rcg);
> 
> This function would retrieve the parent_rate and if the parent_rate is 
> not ready then it would fail to boot up.
> 
> So we have to make sure the parents are registered before these RCGs. 
> That also was one reason for me to not populate the frequency table at 
> recalc.
> 
> We would need this patch to make this work.

Hmmmm. Ok. That won't work then. recalc_rate() better not try to
populate the frequency table then or it will not work. So I suppose it
needs to fallback to reading the registers and assuming the parent_rate
coming in is the actual frequency of it's parent until the frequency
table pointer is non-NULL. Would that work?

BTW, does DFS switch parents without software knowing about it? What
happens in that case? Does the QUP driver make sure that the new parent
of this RCG is properly enabled so that it can switch to it when needed?
I'm still trying to understand this whole design. Who takes care of the
voltage requirements in this case? The QUP driver as well?