Claude review: drm/v3d: Allocate all resources before enabling the clock

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the key restructuring patch. It reorders probe so that resource allocation 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 = 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 = 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` is 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`), the path is `clk_disable` -> `gem_destroy` -> `dma_free`. This ordering is correct: 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