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

[Philipp Zabel]

On Mo, 2025-02-03 at 19:15 +0100, Michal Wilczynski wrote:
> 
> On 1/31/25 16:39, Matt Coster wrote:
> > On 28/01/2025 19:48, Michal Wilczynski wrote:
> > > Add reset controller driver for the T-HEAD TH1520 SoC that manages
> > > hardware reset lines for various subsystems. The driver currently
> > > implements support for GPU reset control, with infrastructure in place
> > > to extend support for NPU and Watchdog Timer resets in future updates.
> > > 
> > > Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> > > ---
> > >  MAINTAINERS                  |   1 +
> > >  drivers/reset/Kconfig        |  10 ++
> > >  drivers/reset/Makefile       |   1 +
> > >  drivers/reset/reset-th1520.c | 178 +++++++++++++++++++++++++++++++++++
> > >  4 files changed, 190 insertions(+)
> > >  create mode 100644 drivers/reset/reset-th1520.c
> > > 
[...]
> > > diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> > > new file mode 100644
> > > index 000000000000..48afbc9f1cdd
> > > --- /dev/null
> > > +++ b/drivers/reset/reset-th1520.c
> > > @@ -0,0 +1,178 @@
[...]
> > > +static void th1520_rst_gpu_enable(struct regmap *reg,
> > > +				  struct mutex *gpu_seq_lock)
> > > +{
> > > +	int val;
> > > +
> > > +	mutex_lock(gpu_seq_lock);
> > > +
> > > +	/* if the GPU is not in a reset state it, put it into one */
> > > +	regmap_read(reg, TH1520_GPU_RST_CFG, &val);
> > > +	if (val)
> > > +		regmap_update_bits(reg, TH1520_GPU_RST_CFG,
> > > +				   TH1520_GPU_RST_CFG_MASK, 0x0);

BIT(2) is not documented, but cleared here.

> > > +
> > > +	/* rst gpu clkgen */
> > > +	regmap_set_bits(reg, TH1520_GPU_RST_CFG, TH1520_GPU_SW_CLKGEN_RST);
> > 
> > Do you know what this resets? From our side, the GPU only has a single
> > reset line (which I assume to be GPU_RESET).
> 
> This is clock generator reset, as described in the manual 5.4.2.6.1
> GPU_RST_CFG. It does reside in the same register as the GPU reset line.
> 
> 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? ...

> > > +
> > > +	/*
> > > +	 * According to the hardware manual, a delay of at least 32 clock
> > > +	 * cycles is required between de-asserting the clkgen reset and
> > > +	 * de-asserting the GPU reset. Assuming a worst-case scenario with
> > > +	 * a very high GPU clock frequency, a delay of 1 microsecond is
> > > +	 * sufficient to ensure this requirement is met across all
> > > +	 * feasible GPU clock speeds.
> > > +	 */
> > > +	udelay(1);
> > 
> > I don't love that this procedure appears in the platform reset driver.
> > I appreciate it may not be clear from the SoC TRM, but this is the
> > standard reset procedure for all IMG Rogue GPUs. The currently
> > supported TI SoC handles this in silicon, when power up/down requests
> > are sent so we never needed to encode it in the driver before.
> > 
> > Strictly speaking, the 32 cycle delay is required between power and
> > clocks being enabled and the reset line being deasserted. If nothing
> > here touches power or clocks (which I don't think it should), the delay
> > could potentially be lifted to the GPU driver.

... This could be expressed as a delay between clk_prepare_enable() and
reset_control_deassert() in the GPU driver then.

> Yeah you're making excellent points here, I think it would be a good    
> idea to place the delay in the GPU driver, since this is specific to the
> whole family of the GPU's not the SoC itself.
>
> > Is it expected that if a device exposes a reset in devicetree that it
> > can be cleanly reset without interaction with the device driver itself?
> > I.E. in this case, is it required that the reset driver alone can cleanly
> > reset the GPU?

No, the "resets" property should just describe the physical
connection(s) between reset controller and the device.

It is fine for the device driver to manually assert the reset, enable
clocks and power, delay, and then deassert the reset, if that is the
device specific reset procedure.

> I'm not sure what the community as a whole thinks about that, so maybe
> someone else can answer this, but I would code SoC specific stuff in the
> reset driver for the SoC, and the GPU specific stuff (like the delay) in
> the GPU driver code. I wasn't sure whether the delay was specific to the
> SoC or the GPU so I've put it here.

I agree.

regards
Philipp