Message-ID: <945fb7e913a9c3dcb40697328b7e9842b75fea5c.camel@pengutronix.de>
Date: Tue, 11 Feb 2025 12:59:39 +0100
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Michal Wilczynski <m.wilczynski@...sung.com>, Matt Coster
 <Matt.Coster@...tec.com>, "mturquette@...libre.com"
 <mturquette@...libre.com>,  "sboyd@...nel.org" <sboyd@...nel.org>,
 "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
 <krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>, 
 "drew@...7.com" <drew@...7.com>, "guoren@...nel.org" <guoren@...nel.org>,
 "wefu@...hat.com" <wefu@...hat.com>, "jassisinghbrar@...il.com"
 <jassisinghbrar@...il.com>,  "paul.walmsley@...ive.com"
 <paul.walmsley@...ive.com>, "palmer@...belt.com" <palmer@...belt.com>, 
 "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>, Frank Binns
 <Frank.Binns@...tec.com>,  "maarten.lankhorst@...ux.intel.com"
 <maarten.lankhorst@...ux.intel.com>, "mripard@...nel.org"
 <mripard@...nel.org>,  "tzimmermann@...e.de" <tzimmermann@...e.de>,
 "airlied@...il.com" <airlied@...il.com>,  "simona@...ll.ch"
 <simona@...ll.ch>, "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>, 
 "jszhang@...nel.org" <jszhang@...nel.org>, "m.szyprowski@...sung.com"
 <m.szyprowski@...sung.com>
Cc: "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, 
	"devicetree@...r.kernel.org"
	 <devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>, "linux-riscv@...ts.infradead.org"
	 <linux-riscv@...ts.infradead.org>, "dri-devel@...ts.freedesktop.org"
	 <dri-devel@...ts.freedesktop.org>, "linux-pm@...r.kernel.org"
	 <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v4 09/18] reset: thead: Add TH1520 reset controller
 driver

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

Powered by blists - more mailing lists