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: clk: keystone: don't cache clock rate
Date: Fri, 27 Feb 2026 14:48:58 +1000
Message-ID: <review-patch5-20260224-gpu_dts-v1-5-cc5ddffe140c@ti.com>
In-Reply-To: <20260224-gpu_dts-v1-5-cc5ddffe140c@ti.com>
References: <20260224-gpu_dts-v1-0-cc5ddffe140c@ti.com> <20260224-gpu_dts-v1-5-cc5ddffe140c@ti.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Author:** Michael Walle (carried by Antonios Christidis)

This patch sets `CLK_GET_RATE_NOCACHE` on all TI SCI clocks because the firmware returns 0 for disabled clocks/consumers, causing the clock framework cache to store an incorrect value.

```c
+	/*
+	 * A clock rate query to the SCI firmware will return 0 if either the
+	 * clock itself is disabled or the attached device/consumer is disabled.
+	 * This makes it inherently unsuitable for the caching of the clk
+	 * framework.
+	 */
+	init.flags = CLK_GET_RATE_NOCACHE;
```

**Good:**
- Well-commented with clear rationale.
- Has three Reviewed-by tags (Kevin Hilman, Randolph Sapp, Nishanth Menon).
- The fix is correct -- if firmware returns 0 for disabled clocks, caching will cause wrong rates to be used after clock enable.

**Concerns:**

1. **Series placement**: This is a bugfix for the clock driver that is a prerequisite for the GPU nodes to work correctly (the GPU driver presumably calls `clk_get_rate()` before enabling the clock). It should be patch 1 in the series, not patch 5, since later patches logically depend on it. Alternatively, it could be submitted as a separate standalone fix with a Fixes: tag, since it affects all SCI clocks, not just GPU clocks.

2. **Potential overwriting of existing flags**: The code does `init.flags = CLK_GET_RATE_NOCACHE;` which *replaces* any existing flags value rather than ORing it in. Looking at the surrounding code, `init.flags` doesn't appear to be set earlier in this function, so this is fine for now, but using `init.flags |= CLK_GET_RATE_NOCACHE;` would be more defensive against future changes adding other flags.

3. **Scope**: This affects *all* TI SCI clocks, not just GPU clocks. The commit message explains why this is necessary (firmware limitation), but the broad scope means this should be tested across the full set of TI K3 platforms to ensure no regressions. Given the existing Reviewed-by tags from TI engineers, this seems to have been vetted.

4. **Missing Fixes: tag**: If this is fixing a bug that existed since the driver was introduced, a `Fixes:` tag referencing the original commit would be appropriate and would help with backporting.

---
Generated by Claude Code Patch Reviewer