[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.gerhold at 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)