Hi Bjorn,

On Tue, 21 Dec 2021 at 00:40, Bjorn Andersson
[off-list ref] wrote:

On Mon 20 Dec 01:54 PST 2021, Loic Poulain wrote:

quoted

When a rcg2 clock migrates to a new parent, both the old and new
parent clocks must be enabled to complete the transition. This can
be automatically performed by the clock core when a clock is flagged
with CLK_OPS_PARENT_ENABLE.

Without this, we may hit rate update failures:
gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
WARNING: CPU: 1 PID: 82 at drivers/clk/qcom/clk-rcg2.c:122 update_config+0xe0/0xf0

Fixes: 496d1a13d405 ("clk: qcom: Add Global Clock Controller driver for QCM2290")
Signed-off-by: Loic Poulain <redacted>
---
 drivers/clk/qcom/gcc-qcm2290.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/clk/qcom/gcc-qcm2290.c b/drivers/clk/qcom/gcc-qcm2290.c
index b6fa7b8..9e1d88e 100644
--- a/drivers/clk/qcom/gcc-qcm2290.c
+++ b/drivers/clk/qcom/gcc-qcm2290.c

@@ -674,6 +674,7 @@ static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
              .name = "gcc_usb30_prim_mock_utmi_clk_src",
              .parent_data = gcc_parents_0,
              .num_parents = ARRAY_SIZE(gcc_parents_0),
+             .flags = CLK_OPS_PARENT_ENABLE,

This seems like a correct fix for the obvious problem that we might end
up invoking clk_set_rate() and clk_set_parent() for these clocks while
their (and thereby themselves - in a software sense) are disabled.


However, clocks that downstream are marked "enable_safe_config", may in
addition be enabled by the hardware, behind out back. As such we must
ensure that they always have a valid configuration, we do this by
"parking" them on CXO whenever we're going to disable their parent
clocks.

Upstream we handle this by using the clk_rcg2_shared_ops, which will do
exactly this.

Ok, thanks for the explanation, so we actually need both
clk_rcg2_shared_ops and CLK_OPS_PARENT_ENABLE, the former parking the
clock under always-on CXO (safe source), allowing hardware to toggle it
on its own. The latter allows safe parent switching by enabling the
new parent clock before update (and old parent, but we don't care
since we are actually parked on CXO) . Is that correct?

PS. Unfortunately these RCGs doesn't allow us to reliably implement
is_enabled() and as such clk_disable_unused() will skip parking the
unused clocks and continue to disable the PLLs feeding them (if they are
unused). I don't think this is a problem for the clocks you list here,
but we certainly have a problem with this in the dispcc. So this is
currently being discussed. For now it's expected that booting without
"clk_ignore_unused" is "broken".

Ok, understood thanks.


Loic