Re: [PATCH] clk: qcom: rcg2: Cache rate changes for parked RCGs

From: Steev Klimaszewski
Date: Sun Dec 12 2021 - 18:28:41 EST



On 12/2/21 9:56 PM, Bjorn Andersson wrote:
As GDSCs are turned on and off some associated clocks are momentarily
enabled for house keeping purposes. Failure to enable these clocks seems
to have been silently ignored in the past, but starting in SM8350 this
failure will prevent the GDSC to turn on.

At least on SM8350 this operation will enable the RCG per the
configuration in CFG_REG. This means that the current model where the
current configuration is written back to CF_REG immediately after
parking the RCG doesn't work.

Instead, keep track of the currently requested rate of the clock and
upon enabling the clock reapply the configuration per the saved rate.

Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed")
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
---
drivers/clk/qcom/clk-rcg.h | 2 ++
drivers/clk/qcom/clk-rcg2.c | 32 +++++++++++++++++---------------
2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index 99efcc7f8d88..6939f4e62768 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -139,6 +139,7 @@ extern const struct clk_ops clk_dyn_rcg_ops;
* @freq_tbl: frequency table
* @clkr: regmap clock handle
* @cfg_off: defines the cfg register offset from the CMD_RCGR + CFG_REG
+ * @current_rate: cached rate for parked RCGs
*/
struct clk_rcg2 {
u32 cmd_rcgr;
@@ -149,6 +150,7 @@ struct clk_rcg2 {
const struct freq_tbl *freq_tbl;
struct clk_regmap clkr;
u8 cfg_off;
+ unsigned long current_rate;
};
#define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr)
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index e1b1b426fae4..b574b38dcbd5 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -167,6 +167,7 @@ clk_rcg2_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;
+ unsigned long rate;
regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg);
@@ -186,7 +187,11 @@ clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
hid_div = cfg >> CFG_SRC_DIV_SHIFT;
hid_div &= mask;
- return calc_rate(parent_rate, m, n, mode, hid_div);
+ rate = calc_rate(parent_rate, m, n, mode, hid_div);
+ if (!rcg->current_rate)
+ rcg->current_rate = rate;
+
+ return rate;
}
static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
@@ -968,12 +973,14 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
if (!f)
return -EINVAL;
+ rcg->current_rate = rate;
+
/*
- * In case clock is disabled, update the CFG, M, N and D registers
- * and don't hit the update bit of CMD register.
+ * In the case that the shared RCG is parked, current_rate will be
+ * applied as the clock is unparked again, so just return here.
*/
if (!__clk_is_enabled(hw->clk))
- return __clk_rcg2_configure(rcg, f);
+ return 0;
return clk_rcg2_shared_force_enable_clear(hw, f);
}
@@ -987,8 +994,13 @@ static int clk_rcg2_shared_set_rate_and_parent(struct clk_hw *hw,
static int clk_rcg2_shared_enable(struct clk_hw *hw)
{
struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+ const struct freq_tbl *f = NULL;
int ret;
+ f = qcom_find_freq(rcg->freq_tbl, rcg->current_rate);
+ if (!f)
+ return -EINVAL;
+
/*
* Set the update bit because required configuration has already
* been written in clk_rcg2_shared_set_rate()
@@ -997,7 +1009,7 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw)
if (ret)
return ret;
- ret = update_config(rcg);
+ ret = clk_rcg2_configure(rcg, f);
if (ret)
return ret;
@@ -1007,13 +1019,6 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw)
static void clk_rcg2_shared_disable(struct clk_hw *hw)
{
struct clk_rcg2 *rcg = to_clk_rcg2(hw);
- u32 cfg;
-
- /*
- * Store current configuration as switching to safe source would clear
- * the SRC and DIV of CFG register
- */
- regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
/*
* Park the RCG at a safe configuration - sourced off of safe source.
@@ -1031,9 +1036,6 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
update_config(rcg);
clk_rcg2_clear_force_enable(hw);
-
- /* Write back the stored configuration corresponding to current rate */
- regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
}
const struct clk_ops clk_rcg2_shared_ops = {

Revisiting this...

With Dmitry's patches applied ( https://lore.kernel.org/linux-arm-msm/20211208022210.1300773-1-dmitry.baryshkov@xxxxxxxxxx/ ) as well as these, and clk_ignore_unused, I get both

[ 4.767487] ------------[ cut here ]------------ [ 4.767495] disp_cc_mdss_pclk0_clk_src: rcg didn't update its configuration.

and


[ 6.449518] ------------[ cut here ]------------ [ 6.449525] video_cc_venus_clk_src: rcg didn't update its configuration.

This includes after modifying Dmitry's patches to park the above 2 clocks.

Removing "clk_ignore_unused" from the kernel command line, while keeping Dmitry's patchset as well as this patch,

results in the disp_cc_mdss_pclk0_clk_src going away, but the video_cc_venus_clk_src still shows up.

Applying Dmitry's patches, removing this one, and removing "clk_ignore_unused" from command line arguments ends up

with none of these rcg didn't update its configuration messages. As can be seen in http://paste.debian.net/1222931

-- steev