public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
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	[thread overview]
Message-ID: <review-patch5-20260224-gpu_dts-v1-5-cc5ddffe140c@ti.com> (raw)
In-Reply-To: <20260224-gpu_dts-v1-5-cc5ddffe140c@ti.com>

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

  reply	other threads:[~2026-02-27  4:48 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-24 18:09 [PATCH 0/5] drm/imagination: add support for j721e,j784s4,j722s,am62p Antonios Christidis
2026-02-24 18:09 ` [PATCH 1/5] arm64: dts: ti: k3-j721e: Add GPU node Antonios Christidis
2026-02-27  4:48   ` Claude review: " Claude Code Review Bot
2026-02-24 18:09 ` [PATCH 2/5] dt-bindings: gpu: img: Add J721e SoC specific compatible Antonios Christidis
2026-02-24 23:09   ` Andrew Davis
2026-02-25 11:06   ` Krzysztof Kozlowski
2026-02-27  4:48   ` Claude review: " Claude Code Review Bot
2026-02-24 18:09 ` [PATCH 3/5] arm64: dts: ti: k3-j784s4: Add GPU node Antonios Christidis
2026-02-27  4:48   ` Claude review: " Claude Code Review Bot
2026-02-24 18:09 ` [PATCH 4/5] arm64: dts: ti: add " a-christidis
2026-02-27  4:48   ` Claude review: " Claude Code Review Bot
2026-02-24 18:09 ` [PATCH 5/5] clk: keystone: don't cache clock rate a-christidis
2026-02-27  4:48   ` Claude Code Review Bot [this message]
2026-02-25  1:05 ` [PATCH 0/5] drm/imagination: add support for j721e,j784s4,j722s,am62p Nishanth Menon
2026-02-27  4:48 ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch5-20260224-gpu_dts-v1-5-cc5ddffe140c@ti.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox