Re: [PATCH v1 4/4] clk: thead: Add GPU clock gate control with CLKGEN reset support

[Stephen Boyd]

Quoting Michal Wilczynski (2025-03-03 06:36:29)
> The T-HEAD TH1520 has three GPU clocks: core, cfg, and mem. The mem
> clock gate is marked as "Reserved" in hardware, while core and cfg are
> configurable. In order for these clock gates to work properly, the
> CLKGEN reset must be managed in a specific sequence.
> 
> Move the CLKGEN reset handling to the clock driver since it's
> fundamentally a clock-related workaround [1]. This ensures that clk_enabled
> GPU clocks stay physically enabled without external interference from
> the reset driver.  The reset is now deasserted only when both core and
> cfg clocks are enabled, and asserted when either of them is disabled.
> 
> The mem clock is configured to use nop operations since it cannot be
> controlled.
> 
> Link: https://lore.kernel.org/all/945fb7e913a9c3dcb40697328b7e9842b75fea5c.camel@xxxxxxxxxxxxxx [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
[...]
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> index ea96d007aecd..1dfcde867233 100644
> --- a/drivers/clk/thead/clk-th1520-ap.c
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -862,17 +863,70 @@ static CCU_GATE(CLK_SRAM1, sram1_clk, "sram1", axi_aclk_pd, 0x20c, BIT(3), 0);
[...]
>  
>  static CCU_GATE_CLK_OPS(CLK_GPU_MEM, gpu_mem_clk, "gpu-mem-clk",
>                         video_pll_clk_pd, 0x0, BIT(2), 0, clk_nop_ops);
> +static CCU_GATE_CLK_OPS(CLK_GPU_CORE, gpu_core_clk, "gpu-core-clk",
> +                       video_pll_clk_pd, 0x0, BIT(3), 0, ccu_gate_gpu_ops);
> +static CCU_GATE_CLK_OPS(CLK_GPU_CFG_ACLK, gpu_cfg_aclk, "gpu-cfg-aclk",
> +                       video_pll_clk_pd, 0x0, BIT(4), 0, ccu_gate_gpu_ops);
> +
> +static void ccu_gpu_clk_disable(struct clk_hw *hw)
> +{
> +       struct ccu_gate *cg = hw_to_ccu_gate(hw);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gpu_reset_lock, flags);
> +
> +       ccu_disable_helper(&cg->common, cg->enable);
> +
> +       if ((cg == &gpu_core_clk &&
> +            !clk_hw_is_enabled(&gpu_cfg_aclk.common.hw)) ||
> +           (cg == &gpu_cfg_aclk &&
> +            !clk_hw_is_enabled(&gpu_core_clk.common.hw)))
> +               reset_control_assert(gpu_reset);

Why can't the clk consumer control the reset itself? Doing this here is
not ideal because we hold the clk lock when we try to grab the reset
lock. These are all spinlocks that should be small in lines of code
where the lock is held, but we're calling into an entire other framework
under a spinlock. If an (unrelated) reset driver tries to grab the clk
lock it will deadlock.

I see the commit text talks about this being a workaround. I'm not sure
what the workaround is though. I've seen designs where the reset doesn't
work unless the clk is enabled because the flops have to be clocking for
the reset to propagate a few cycles, or the clk has to be disabled so
that the reset controller can do the clocking, or vice versa for the clk
not working unless the reset is deasserted. Long story short, it's
different between SoCs.

Likely the reset and clk control should be coordinated in a PM domain
for the device so that when the device is active, the clks are enabled
and the reset is deasserted in the correct order for the SoC. Can you do
that?

> +
> +       spin_unlock_irqrestore(&gpu_reset_lock, flags);
> +}