Re: [PATCH 4/6] clk: qcom: Allow polling for clock status in qcom_gate_clk_en()

[Casey Connolly]

On 4/22/25 12:54, Stephan Gerhold wrote:
GATE_CLK() in its current state is unsafe: A simple write to the clock
enable register does not guarantee that the clock is immediately running.
Without polling the clock status, we may issue writes to registers before
the necessary clocks start running. This doesn't seem to cause issues in
U-Boot at the moment, but for example removing the CLK_OFF polling in TF-A
for the SMMU clocks on DB410c reliably triggers an exception during boot.

Make it possible to poll the branch clock status register, by adding a new
BRANCH_CLK() macro that takes the extra register address. Existing usages
work just as before, without polling the clock status. Ideally all usages
should be updated to specify the correct poll address in the future.

Signed-off-by: Stephan Gerhold <stephan.gerh...@linaro.org>
---
  drivers/clk/qcom/clock-qcom.c | 15 +++++++++++++++
  drivers/clk/qcom/clock-qcom.h |  7 +++++--
  2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c
index 
7a259db79342c31dfe85220c68c82197861ccdf6..6b46d9db744b87538b796bf729d16785bca08545
 100644
--- a/drivers/clk/qcom/clock-qcom.c
+++ b/drivers/clk/qcom/clock-qcom.c
@@ -83,6 +83,21 @@ int qcom_gate_clk_en(const struct msm_clk_priv *priv, 
unsigned long id)
        }
  
  	setbits_le32(priv->base + priv->data->clks[id].reg, priv->data->clks[id].en_val);
+       if (priv->data->clks[id].cbcr_reg) {
+               unsigned int count;
+               u32 val;
+
+               for (count = 0; count < 200; count++) {
+                       val = readl(priv->base + priv->data->clks[id].cbcr_reg);
+                       val &= BRANCH_CHECK_MASK;
+                       if (val == BRANCH_ON_VAL || val == 
BRANCH_NOC_FSM_ON_VAL)
+                               break;
+                       udelay(1);
+               }
+               if (WARN(count == 200, "WARNING: Clock @ %#lx [%#010x] stuck at 
off\n",
+                        priv->data->clks[id].cbcr_reg, val))
+                       return -EBUSY;
+       }
        return 0;
  }
  
diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h
index 
ee0347d9d86716c09e7f57b343f12b5908f8cb92..28145061844255460e8a6868b69ffe9a7dd663b0
 100644
--- a/drivers/clk/qcom/clock-qcom.h
+++ b/drivers/clk/qcom/clock-qcom.h
@@ -52,13 +52,16 @@ struct freq_tbl {
  struct gate_clk {
        uintptr_t reg;
        u32 en_val;
+       uintptr_t cbcr_reg;
        const char *name;
  };
  
  #ifdef DEBUG
-#define GATE_CLK(clk, reg, val) [clk] = { reg, val, #clk }
+#define GATE_CLK(clk, reg, val) [clk] = { reg, val, 0, #clk }
+#define BRANCH_CLK(clk, en_reg, val, cbcr_reg) [clk] = { en_reg, val, 
cbcr_reg, #clk }

The "gate clock" naming is kinda BS here anyways, I think it's confusing 
to have "gate clock" = "enable and run" and "branch clock" = "wait for 
enable"

We should probably rename gate clock to branch clock, but let's not 
block this series on that. For now would you be fine with renaming the 
"BRANCH_CLK" macro to "GATE_CLK_POLLED"? Maybe add a comment that we 
plan to rename GATE->BRANCH and that using GATE_CLK (non-polled) is 
deprecated and the polled variant will be required for future patches.

Kind regards,>   #else
-#define GATE_CLK(clk, reg, val) [clk] = { reg, val, NULL }
+#define GATE_CLK(clk, reg, val) [clk] = { reg, val, 0, NULL }
+#define BRANCH_CLK(clk, en_reg, val, cbcr_reg) [clk] = { en_reg, val, 
cbcr_reg, NULL }
  #endif
  
  struct qcom_reset_map {


--
Casey (she/they)