From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject:
 Claude review: drm/v3d: Allocate all resources before enabling the clock
Date: Sat, 14 Feb 2026 07:21:53 +1000
Message-ID:
 <review-patch6-20260213-v3d-power-management-v5-6-7a8b381eb379@igalia.com>
In-Reply-To: <20260213-v3d-power-management-v5-6-7a8b381eb379@igalia.com>
References: <20260213-v3d-power-management-v5-0-7a8b381eb379@igalia.com>
 <20260213-v3d-power-management-v5-6-7a8b381eb379@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the key restructuring patch. It reorders probe so that resource alloc=
ation happens before clock enable, and hardware initialization happens after.

Looking at the new probe ordering:
1. Register mappings (hub, core0, sms, gca, bridge)
2. Reset control and clock acquisition
3. `v3d_irq_init()` -- requests IRQs but no longer clears pending or enables
4. `v3d_perfmon_init()`
5. MMU scratch page allocation
6. `v3d_gem_init()` -- no longer does HW init
7. `clk_prepare_enable()` -- hardware access begins here
8. SMS idle, HW ident reads, `v3d_init_hw_state()`, `v3d_mmu_set_page_table()=
`, `v3d_irq_enable()`
9. `drm_dev_register()`

The separation of `v3d_irq_init()` (IRQ request only) from `v3d_irq_enable()`=
 (HW register writes) is well done:

> -	/* Clear any pending interrupts someone might have left around
> -	 * for us.
> -	 */
> -	for (core =3D 0; core < v3d->cores; core++)
> -		V3D_CORE_WRITE(core, V3D_CTL_INT_CLR, V3D_CORE_IRQS(v3d->ver));
> -	V3D_WRITE(V3D_HUB_INT_CLR, V3D_HUB_IRQS(v3d->ver));

This pending interrupt clear is moved into `v3d_irq_enable()`:

> +	/* Clear any pending interrupts someone might have left around for us. */
> +	for (core =3D 0; core < v3d->cores; core++)
> +		V3D_CORE_WRITE(core, V3D_CTL_INT_CLR, V3D_CORE_IRQS(v3d->ver));
> +	V3D_WRITE(V3D_HUB_INT_CLR, V3D_HUB_IRQS(v3d->ver));

This is correct because `v3d_irq_enable()` is now called after `v3d->cores` i=
s set from the hardware ident register.

However, there is a concern about the error cleanup path:

> +clk_disable:
> +	v3d_power_off_sms(v3d);
> +	clk_disable_unprepare(v3d->clk);
>  gem_destroy:
>  	v3d_gem_destroy(drm);
>  dma_free:
>  	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
> -clk_disable:
> -	clk_disable_unprepare(v3d->clk);
>  	return ret;

If `drm_dev_register()` or `v3d_sysfs_init()` fails, the error path jumps to =
`irq_disable` then falls through to `clk_disable`, `gem_destroy`, `dma_free`.=
 But if `dma_set_mask_and_coherent()` fails (which goes to `clk_disable`), th=
e path is `clk_disable` -> `gem_destroy` -> `dma_free`. This ordering is corr=
ect: clock off, then gem destroy, then free DMA memory.

The remove function ordering also matches:

> +	v3d_power_off_sms(v3d);
> +	clk_disable_unprepare(v3d->clk);
> +	v3d_gem_destroy(drm);
> +	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);

This looks correct -- power off before freeing software resources.

One small note: there's a double blank line after the new `v3d_init_hw_state(=
)` function definition in v3d_gem.c:

> +void
> +v3d_init_hw_state(struct v3d_dev *v3d)
> +{
> +	v3d_init_core(v3d, 0);
> +}
> +
> +

Minor style nit, not a correctness issue.

---
Generated by Claude Code Patch Reviewer