On 09/12/2021 21:36, Bjorn Andersson wrote:
On Thu 09 Dec 00:37 PST 2021, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2021-12-07 18:22:09)
Some of RCG2 clocks can become stuck during the boot process, when
device drivers are enabling and disabling the RCG2's parent clocks.
To prevernt such outcome of driver probe sequences, add API to park
s/prevernt/prevent/
clocks to the safe clock source (typically TCXO).
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
I'd prefer this approach vs. adding a new clk flag. The clk framework
doesn't handle handoff properly today so we shouldn't try to bandage
that in the core.
I'm not against putting this responsibility in the drivers, but I don't
think we can blindly park all the RCGs that may or may not be used.
Note that we should do this for all RCGs that downstream are marked as
enable_safe_config (upstream should be using clk_rcg2_shared_ops)
and disabling some of those probe time won't be appreciated by the
hardware.
Only for the hardware as crazy, as displays. And maybe gmu_clk_src. I
don't think we expect venus or camcc to be really clocking when kernel
boots.
If you don't like the flag passed to clk_disable_unused (which is like a
very reasonable objection to have), we need to make progress towards a
proper solution that replaces clk_disable_unused().
The issue is that at the time of clk_disable_unused() it can be too
late, for example because msm being built-in into the kernel has already
tried to play with PLLs/GDSCs and thus made RCG stuck. This is what I
was observing on RB3 if the msm driver is built in and the splash screen
is enabled.
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index e1b1b426fae4..230b04a7427c 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -1036,6 +1036,40 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
}
+int clk_rcg2_park_safely(struct regmap *regmap, u32 offset, unsigned int safe_src)
This seems to just duplicate clk_rcg2_shared_disable()?
A light version of it. It does not do force_on/_off. And also it can not
rely on clkr->regmap or clock name being set. Initially I used
clk_rcg2_shared_disable + several patches to stop it from crashing if it
is used on the non-registered clock. Then I just decided to write
special helper.
Regards,
Bjorn
Please add kernel doc as it's an exported symbol.
Ack
+{
+ unsigned int val, ret, count;
+
+ ret = regmap_read(regmap, offset + CFG_REG, &val);
+ if (ret)
+ return ret;
+
+ /* assume safe source is 0 */
Are we assuming safe source is 0 here? It looks like we pass it in now?
Leftover, will remove if/when posting v2.
+ if ((val & CFG_SRC_SEL_MASK) == (safe_src << CFG_SRC_SEL_SHIFT))
+ return 0;
+
+ regmap_write(regmap, offset + CFG_REG, safe_src << CFG_SRC_SEL_SHIFT);
+
+ ret = regmap_update_bits(regmap, offset + CMD_REG,
+ CMD_UPDATE, CMD_UPDATE);
+ if (ret)
+ return ret;
+
+ /* Wait for update to take effect */
+ for (count = 500; count > 0; count--) {
+ ret = regmap_read(regmap, offset + CMD_REG, &val);
+ if (ret)
+ return ret;
+ if (!(val & CMD_UPDATE))
+ return 0;
+ udelay(1);
+ }
+
+ WARN(1, "the rcg didn't update its configuration.");
Add a newline?
Ack.
+ return -EBUSY;
+}
+EXPORT_SYMBOL_GPL(clk_rcg2_park_safely);
+
--
With best wishes
Dmitry
