[PATCH v4 09/18] reset: thead: Add TH1520 reset controller driver

[Philipp Zabel]

On Mo, 2025-02-10 at 19:17 +0100, Michal Wilczynski wrote:
> On 2/4/25 18:18, Philipp Zabel wrote:
> > On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote:
[...]
> > > I think this is required because the MEM clock gate is somehow broken
> > > and marked as 'reserved' in manual, so instead as a workaround, since we
> > > can't reliably enable the 'mem' clock it's a good idea to reset the
> > > whole CLKGEN of the GPU.
> > 
> > If this is a workaround for broken gating of the "mem" clock, would it
> > be possible (and reasonable) to make this a separate reset control that
> > is handled by the clock driver? ...
> 
> Thank you for the detailed feedback, Philipp.
> 
> After further consideration, I believe keeping the current reset driver
> implementation would be preferable to moving the CLKGEN reset handling
> to the clock driver. While it's technically possible to implement this
> in the clock driver, I have concerns about the added complexity:
> 
> 1. We'd need to expose the CLKGEN reset separately in the reset driver

I'd expect this to simplify the reset driver.

> 2. The clock driver's dt-bindings would need modification to add an
>    optional resets property

If it describes the hardware correctly, that should be fine.

> 3. We'd need custom clk_ops for all three clock gates (including a dummy
>    'mem' gate)
> 4. Each clock gate's .enable operation would need to handle CLKGEN reset
>    deassertion

I accept these arguments, as I have no good feeling for how much
complexity this would actually add.

In my mind it shouldn't be much: the GPU clocks could all share the
same refcounted implementation. The first clock to get enabled would
ungate both GPU_CORE and GPU_CFG_ACLK gates and deassert
GPU_SW_CLKGEN_RST, all in one place. The remaining enable(s) would be
no-ops. Would that work?

Whether a separate "dummy" MEM clock for the DT bindings is added or
not would not make a difference.

> While the clock framework could theoretically handle this, there's no
> clean way to express the requirement that the CLKGEN reset should only
> be deasserted after all clocks in the group are enabled. We could
> implement this explicitly, but it would make the code more complex and
> harder to understand.

Doing this in the clock driver would have the advantage of clk_enabled
GPU clocks actually staying physically enabled, without the reset
driver disabling them via GPU_SW_CLKGEN_RST from the outside.

> The current solution in the reset driver is simpler and clearer - it
> treats this as what it really is: a TH1520-specific reset sequence.

Yes. What this also is: a workaround for a SoC specific defect in the
clock tree. I think it belongs in the clock driver because of this.


[...]
> Regarding the delay between clock enable and reset deassert - for SoCs
> like BPI-F3 with a single reset line, implementing this in the GPU
> consumer driver makes perfect sense. However, for the T-HEAD SoC, moving
> the delay there would actually complicate things since we need to manage
> both the CLKGEN and GPU reset lines in a specific sequence. Having this
> handled entirely within the reset driver keeps the implementation
> cleaner.

You could delay in both places, it's just a microsecond after all.
Whether the workaround is implemented in the reset driver or in the
clock driver, I wouldn't want the GPU driver to have to carry a special
case for TH1520.

> Does this reasoning align with your thoughts? I'm happy to explore the
> clock driver approach further if you still see significant advantages to
> that solution.

I won't object to carry this in the reset driver if the clock
implementation turns out to be unreasonably complex, but I currently
don't expect that to be the case. Please give it a shot.

regards
Philipp