| Message ID | 20211203035601.3505780-1-bjorn.andersson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: rcg2: Cache rate changes for parked RCGs | expand |
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@linaro.org> > --- > 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@linaro.org/ ) 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
On Thu, 2 Dec 2021 19:56:01 -0800, 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. > > [...] Applied, thanks! [1/1] clk: qcom: rcg2: Cache rate changes for parked RCGs commit: ce4981ad69145d7444a5cf4fef2e29442aee60dc Best regards,
Quoting Bjorn Andersson (2021-12-02 19:56:01) > 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. Just to clarify, is the RCG off and "parked" at XO with the config register dirty and set to the desired frequency and then the RCG is turned on by the GDSC? > > Instead, keep track of the currently requested rate of the clock and > upon enabling the clock reapply the configuration per the saved rate. We already keep track of the requested rate and reapply it on enable, just we're lazy and stash that information in the hardware and not the software. I didn't think the gdsc would be turned on and ruin that all, but it's fair. > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > 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; Instead of doing this in recalc_rate, all the time, why not make an init clk op that does it once during registration? The other problem I see is that the rate we calculate may be wrong if the parent is registered after this clk. I think this came up originally when the patch this is fixing was discussed. So instead of saving the current_rate can we save the cfg register value (or however many registers we need) to put back the frequency of the clk to what we want on enable? The other thing is that we made recalc_rate() work "seamlessly" here by stashing the frequency into the register but leaving it uncommitted until enable. We may need to now look at the software copy of the registers in the shared rcg recalc rate operation to figure out what the frequency is. > + > + 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; Do not assign a value to a variable > int ret; > > + f = qcom_find_freq(rcg->freq_tbl, rcg->current_rate); and then assign it again without testing it before. > + if (!f) > + return -EINVAL; > + > /* > * Set the update bit because required configuration has already
On Wed 15 Dec 17:51 PST 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-12-02 19:56:01) > > 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. > > Just to clarify, is the RCG off and "parked" at XO with the config > register dirty and set to the desired frequency and then the RCG is > turned on by the GDSC? > Correct, that's exactly what I'm observing. > > > > Instead, keep track of the currently requested rate of the clock and > > upon enabling the clock reapply the configuration per the saved rate. > > We already keep track of the requested rate and reapply it on enable, > just we're lazy and stash that information in the hardware and not the > software. I didn't think the gdsc would be turned on and ruin that all, > but it's fair. > Up until SM8350 I see no evidence that this has been a problem, but now it is. So there's likely some changes in the hardware there... > > > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > 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; > > Instead of doing this in recalc_rate, all the time, why not make an init > clk op that does it once during registration? The other problem I see is > that the rate we calculate may be wrong if the parent is registered > after this clk. I think this came up originally when the patch this is > fixing was discussed. > I would need to go back and reproduce the issue I saw, but I had to add this because I ended up in clk_rcg2_shared_enable() with current_rate = 0, which I think would be equally bad to just committing the dirty configuration. > So instead of saving the current_rate can we save the cfg register value > (or however many registers we need) to put back the frequency of the clk > to what we want on enable? The other thing is that we made recalc_rate() > work "seamlessly" here by stashing the frequency into the register but > leaving it uncommitted until enable. We may need to now look at the > software copy of the registers in the shared rcg recalc rate operation > to figure out what the frequency is. > I made an attempt at this, the problem I had was to come up within something sane for how to deal with set_rate on parked clocks; because we need to re-generate the register contents, without writing out the value - and that got messy. So stashing the frequency turned out to be much cleaner. I believe that this is also what they do downstream... Regards, Bjorn
Quoting Bjorn Andersson (2021-12-15 18:48:27) > On Wed 15 Dec 17:51 PST 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-12-02 19:56:01) > > > 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. > > > > Just to clarify, is the RCG off and "parked" at XO with the config > > register dirty and set to the desired frequency and then the RCG is > > turned on by the GDSC? > > > > Correct, that's exactly what I'm observing. Cool can you add that detail to the commit message? > > > > > > > Instead, keep track of the currently requested rate of the clock and > > > upon enabling the clock reapply the configuration per the saved rate. > > > > We already keep track of the requested rate and reapply it on enable, > > just we're lazy and stash that information in the hardware and not the > > software. I didn't think the gdsc would be turned on and ruin that all, > > but it's fair. > > > > Up until SM8350 I see no evidence that this has been a problem, but now > it is. So there's likely some changes in the hardware there... > > > > > > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > 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; > > > > Instead of doing this in recalc_rate, all the time, why not make an init > > clk op that does it once during registration? The other problem I see is > > that the rate we calculate may be wrong if the parent is registered > > after this clk. I think this came up originally when the patch this is > > fixing was discussed. > > > > I would need to go back and reproduce the issue I saw, but I had to add > this because I ended up in clk_rcg2_shared_enable() with current_rate = > 0, which I think would be equally bad to just committing the dirty > configuration. Alright. > > > So instead of saving the current_rate can we save the cfg register value > > (or however many registers we need) to put back the frequency of the clk > > to what we want on enable? The other thing is that we made recalc_rate() > > work "seamlessly" here by stashing the frequency into the register but > > leaving it uncommitted until enable. We may need to now look at the > > software copy of the registers in the shared rcg recalc rate operation > > to figure out what the frequency is. > > > > I made an attempt at this, the problem I had was to come up within > something sane for how to deal with set_rate on parked clocks; because > we need to re-generate the register contents, without writing out the > value - and that got messy. Looking back on the introduction of this code[1] I see that it's not about the rate but more about the parent. i.e. we park the clk on the XO parent but don't care about the m/n values or pre divider because it doesn't really matter if the clk is running slowly. So nothing needs to be saved except for the cfg register, and we can do that in software with a single u32 instead of using a rate and looking it up and then reprogramming the other values. We should be able to cache the register content with an init clk_op. > > So stashing the frequency turned out to be much cleaner. I believe that > this is also what they do downstream... > Downstream doing something isn't a great reason. [1] https://lore.kernel.org/r/07dbf890975c1178bc4cc2360c25526a@codeaurora.org
On Thu 16 Dec 10:58 PST 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-12-15 18:48:27) > > On Wed 15 Dec 17:51 PST 2021, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2021-12-02 19:56:01) > > > > 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. > > > > > > Just to clarify, is the RCG off and "parked" at XO with the config > > > register dirty and set to the desired frequency and then the RCG is > > > turned on by the GDSC? > > > > > > > Correct, that's exactly what I'm observing. > > Cool can you add that detail to the commit message? > Sure. > > > > > > > > > > Instead, keep track of the currently requested rate of the clock and > > > > upon enabling the clock reapply the configuration per the saved rate. > > > > > > We already keep track of the requested rate and reapply it on enable, > > > just we're lazy and stash that information in the hardware and not the > > > software. I didn't think the gdsc would be turned on and ruin that all, > > > but it's fair. > > > > > > > Up until SM8350 I see no evidence that this has been a problem, but now > > it is. So there's likely some changes in the hardware there... > > > > > > > > > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > --- > > > > 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; > > > > > > Instead of doing this in recalc_rate, all the time, why not make an init > > > clk op that does it once during registration? The other problem I see is > > > that the rate we calculate may be wrong if the parent is registered > > > after this clk. I think this came up originally when the patch this is > > > fixing was discussed. > > > > > > > I would need to go back and reproduce the issue I saw, but I had to add > > this because I ended up in clk_rcg2_shared_enable() with current_rate = > > 0, which I think would be equally bad to just committing the dirty > > configuration. > > Alright. > > > > > > So instead of saving the current_rate can we save the cfg register value > > > (or however many registers we need) to put back the frequency of the clk > > > to what we want on enable? The other thing is that we made recalc_rate() > > > work "seamlessly" here by stashing the frequency into the register but > > > leaving it uncommitted until enable. We may need to now look at the > > > software copy of the registers in the shared rcg recalc rate operation > > > to figure out what the frequency is. > > > > > > > I made an attempt at this, the problem I had was to come up within > > something sane for how to deal with set_rate on parked clocks; because > > we need to re-generate the register contents, without writing out the > > value - and that got messy. > > Looking back on the introduction of this code[1] I see that it's not > about the rate but more about the parent. i.e. we park the clk on the XO > parent but don't care about the m/n values or pre divider because it > doesn't really matter if the clk is running slowly. So nothing needs to > be saved except for the cfg register, and we can do that in software > with a single u32 instead of using a rate and looking it up and then > reprogramming the other values. We should be able to cache the register > content with an init clk_op. > So you're suggesting that, in clk_rcg2_shared_set_rate(), when the RCG is found to be disabled, I should write out M, N, D and calculate a new cfg value which I stash until the next enable? Looks a little bit messy, but I will give it a try. > > > > So stashing the frequency turned out to be much cleaner. I believe that > > this is also what they do downstream... > > > > Downstream doing something isn't a great reason. > :) > [1] https://lore.kernel.org/r/07dbf890975c1178bc4cc2360c25526a@codeaurora.org Regards, Bjorn
Quoting Bjorn Andersson (2021-12-16 15:32:32) > On Thu 16 Dec 10:58 PST 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-12-15 18:48:27) > > > On Wed 15 Dec 17:51 PST 2021, Stephen Boyd wrote: > > > > > > > Quoting Bjorn Andersson (2021-12-02 19:56:01) > > > > > 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. > > > > > > > > Just to clarify, is the RCG off and "parked" at XO with the config > > > > register dirty and set to the desired frequency and then the RCG is > > > > turned on by the GDSC? > > > > > > > > > > Correct, that's exactly what I'm observing. > > > > Cool can you add that detail to the commit message? > > > > Sure. > > > > > > > > > > > > > > Instead, keep track of the currently requested rate of the clock and > > > > > upon enabling the clock reapply the configuration per the saved rate. > > > > > > > > We already keep track of the requested rate and reapply it on enable, > > > > just we're lazy and stash that information in the hardware and not the > > > > software. I didn't think the gdsc would be turned on and ruin that all, > > > > but it's fair. > > > > > > > > > > Up until SM8350 I see no evidence that this has been a problem, but now > > > it is. So there's likely some changes in the hardware there... > > > > > > > > > > > > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > --- > > > > > 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; > > > > > > > > Instead of doing this in recalc_rate, all the time, why not make an init > > > > clk op that does it once during registration? The other problem I see is > > > > that the rate we calculate may be wrong if the parent is registered > > > > after this clk. I think this came up originally when the patch this is > > > > fixing was discussed. > > > > > > > > > > I would need to go back and reproduce the issue I saw, but I had to add > > > this because I ended up in clk_rcg2_shared_enable() with current_rate = > > > 0, which I think would be equally bad to just committing the dirty > > > configuration. > > > > Alright. > > > > > > > > > So instead of saving the current_rate can we save the cfg register value > > > > (or however many registers we need) to put back the frequency of the clk > > > > to what we want on enable? The other thing is that we made recalc_rate() > > > > work "seamlessly" here by stashing the frequency into the register but > > > > leaving it uncommitted until enable. We may need to now look at the > > > > software copy of the registers in the shared rcg recalc rate operation > > > > to figure out what the frequency is. > > > > > > > > > > I made an attempt at this, the problem I had was to come up within > > > something sane for how to deal with set_rate on parked clocks; because > > > we need to re-generate the register contents, without writing out the > > > value - and that got messy. > > > > Looking back on the introduction of this code[1] I see that it's not > > about the rate but more about the parent. i.e. we park the clk on the XO > > parent but don't care about the m/n values or pre divider because it > > doesn't really matter if the clk is running slowly. So nothing needs to > > be saved except for the cfg register, and we can do that in software > > with a single u32 instead of using a rate and looking it up and then > > reprogramming the other values. We should be able to cache the register > > content with an init clk_op. > > > > So you're suggesting that, in clk_rcg2_shared_set_rate(), when the RCG > is found to be disabled, I should write out M, N, D and calculate a new > cfg value which I stash until the next enable? > > Looks a little bit messy, but I will give it a try. No. I don't see where clk_rcg2_shared_set_rate() needs to change. I'm suggesting we cache the config register on disable so it can be restored on enable. Basically everything is the same except now we don't write the cfg register and leave it dirty in the hardware. We need a shared rcg version of recalc rate that looks at the shadow cfg register instead of reading the hardware because we've changed the parent behind the back of the framework and we want to make it look like nothing has changed. This is all based on my understanding that the problem is the RCG is changing rate due to the gdsc turning on the clk for us. So we can't leave anything dirty in the hardware and have to keep it in software. I hope the change is minimal.
On Thu 16 Dec 19:45 CST 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-12-16 15:32:32) > > On Thu 16 Dec 10:58 PST 2021, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2021-12-15 18:48:27) > > > > On Wed 15 Dec 17:51 PST 2021, Stephen Boyd wrote: > > > > > > > > > Quoting Bjorn Andersson (2021-12-02 19:56:01) > > > > > > 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. > > > > > > > > > > Just to clarify, is the RCG off and "parked" at XO with the config > > > > > register dirty and set to the desired frequency and then the RCG is > > > > > turned on by the GDSC? > > > > > > > > > > > > > Correct, that's exactly what I'm observing. > > > > > > Cool can you add that detail to the commit message? > > > > > > > Sure. > > > > > > > > > > > > > > > > > > Instead, keep track of the currently requested rate of the clock and > > > > > > upon enabling the clock reapply the configuration per the saved rate. > > > > > > > > > > We already keep track of the requested rate and reapply it on enable, > > > > > just we're lazy and stash that information in the hardware and not the > > > > > software. I didn't think the gdsc would be turned on and ruin that all, > > > > > but it's fair. > > > > > > > > > > > > > Up until SM8350 I see no evidence that this has been a problem, but now > > > > it is. So there's likely some changes in the hardware there... > > > > > > > > > > > > > > > > Fixes: 7ef6f11887bd ("clk: qcom: Configure the RCGs to a safe source as needed") > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > > --- > > > > > > 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; > > > > > > > > > > Instead of doing this in recalc_rate, all the time, why not make an init > > > > > clk op that does it once during registration? The other problem I see is > > > > > that the rate we calculate may be wrong if the parent is registered > > > > > after this clk. I think this came up originally when the patch this is > > > > > fixing was discussed. > > > > > > > > > > > > > I would need to go back and reproduce the issue I saw, but I had to add > > > > this because I ended up in clk_rcg2_shared_enable() with current_rate = > > > > 0, which I think would be equally bad to just committing the dirty > > > > configuration. > > > > > > Alright. > > > > > > > > > > > > So instead of saving the current_rate can we save the cfg register value > > > > > (or however many registers we need) to put back the frequency of the clk > > > > > to what we want on enable? The other thing is that we made recalc_rate() > > > > > work "seamlessly" here by stashing the frequency into the register but > > > > > leaving it uncommitted until enable. We may need to now look at the > > > > > software copy of the registers in the shared rcg recalc rate operation > > > > > to figure out what the frequency is. > > > > > > > > > > > > > I made an attempt at this, the problem I had was to come up within > > > > something sane for how to deal with set_rate on parked clocks; because > > > > we need to re-generate the register contents, without writing out the > > > > value - and that got messy. > > > > > > Looking back on the introduction of this code[1] I see that it's not > > > about the rate but more about the parent. i.e. we park the clk on the XO > > > parent but don't care about the m/n values or pre divider because it > > > doesn't really matter if the clk is running slowly. So nothing needs to > > > be saved except for the cfg register, and we can do that in software > > > with a single u32 instead of using a rate and looking it up and then > > > reprogramming the other values. We should be able to cache the register > > > content with an init clk_op. > > > > > > > So you're suggesting that, in clk_rcg2_shared_set_rate(), when the RCG > > is found to be disabled, I should write out M, N, D and calculate a new > > cfg value which I stash until the next enable? > > > > Looks a little bit messy, but I will give it a try. > > No. I don't see where clk_rcg2_shared_set_rate() needs to change. > > I'm suggesting we cache the config register on disable so it can be > restored on enable. Basically everything is the same except now we don't > write the cfg register and leave it dirty in the hardware. We need a > shared rcg version of recalc rate that looks at the shadow cfg register > instead of reading the hardware because we've changed the parent behind > the back of the framework and we want to make it look like nothing has > changed. > I see, that was my first attempt of an implementation as well. The problem I ran into right away was that i had something that did disable(), set_rate(), enable() and I would restore the wrong settings. So clk_rcg2_shared_set_rate() needs to update the stashed cfg value - and it needs to write out M, N and D if we're not caching those. > This is all based on my understanding that the problem is the RCG is > changing rate due to the gdsc turning on the clk for us. So we can't > leave anything dirty in the hardware and have to keep it in software. > I hope the change is minimal. That's my understanding as well. Looking more at the code I think it's possible that we get disable(), set_parent(), enable() as well; which if that's the case would result in the same problem, so I assume I need to tend to that as well. Regards, Bjorn
Quoting Bjorn Andersson (2021-12-16 20:05:52) > On Thu 16 Dec 19:45 CST 2021, Stephen Boyd wrote: > > > > > > > > > > > > > > > So instead of saving the current_rate can we save the cfg register value > > > > > > (or however many registers we need) to put back the frequency of the clk > > > > > > to what we want on enable? The other thing is that we made recalc_rate() > > > > > > work "seamlessly" here by stashing the frequency into the register but > > > > > > leaving it uncommitted until enable. We may need to now look at the > > > > > > software copy of the registers in the shared rcg recalc rate operation > > > > > > to figure out what the frequency is. > > > > > > > > > > > > > > > > I made an attempt at this, the problem I had was to come up within > > > > > something sane for how to deal with set_rate on parked clocks; because > > > > > we need to re-generate the register contents, without writing out the > > > > > value - and that got messy. > > > > > > > > Looking back on the introduction of this code[1] I see that it's not > > > > about the rate but more about the parent. i.e. we park the clk on the XO > > > > parent but don't care about the m/n values or pre divider because it > > > > doesn't really matter if the clk is running slowly. So nothing needs to > > > > be saved except for the cfg register, and we can do that in software > > > > with a single u32 instead of using a rate and looking it up and then > > > > reprogramming the other values. We should be able to cache the register > > > > content with an init clk_op. > > > > > > > > > > So you're suggesting that, in clk_rcg2_shared_set_rate(), when the RCG > > > is found to be disabled, I should write out M, N, D and calculate a new > > > cfg value which I stash until the next enable? > > > > > > Looks a little bit messy, but I will give it a try. > > > > No. I don't see where clk_rcg2_shared_set_rate() needs to change. > > > > I'm suggesting we cache the config register on disable so it can be > > restored on enable. Basically everything is the same except now we don't > > write the cfg register and leave it dirty in the hardware. We need a > > shared rcg version of recalc rate that looks at the shadow cfg register > > instead of reading the hardware because we've changed the parent behind > > the back of the framework and we want to make it look like nothing has > > changed. > > > > I see, that was my first attempt of an implementation as well. > > The problem I ran into right away was that i had something that did > disable(), set_rate(), enable() and I would restore the wrong settings. > > So clk_rcg2_shared_set_rate() needs to update the stashed cfg value - > and it needs to write out M, N and D if we're not caching those. > > > This is all based on my understanding that the problem is the RCG is > > changing rate due to the gdsc turning on the clk for us. So we can't > > leave anything dirty in the hardware and have to keep it in software. > > I hope the change is minimal. > > That's my understanding as well. > > > Looking more at the code I think it's possible that we get disable(), > set_parent(), enable() as well; which if that's the case would result > in the same problem, so I assume I need to tend to that as well. Hopefully we can write the M, N, D and pre-divier values all the time and avoid writing the parent selection register (the cfg one) when the clk is off. It would mean that the "safe parent" of XO is getting divided pretty heavily sometimes but I suspect it doesn't matter in practice. When the clk is enabled in the kernel, we'll move the parent over by making the cfg register have the right parent selectoin and then the mnd and divider would already be what they're supposed to be in the hardware. Does that work?
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 = {
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@linaro.org> --- drivers/clk/qcom/clk-rcg.h | 2 ++ drivers/clk/qcom/clk-rcg2.c | 32 +++++++++++++++++--------------- 2 files changed, 19 insertions(+), 15 deletions(-)