[PATCH v2 0/2] drm/imagination: Improve power off sequence

[PATCH v2 0/2] drm/imagination: Improve power off sequence

[Brajesh Gupta]

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
Changes in v2:
- Patch updates as per feedback
- Link to v1: https://lore.kernel.org/r/20260305-b4-staging-layout_mars_base-v1-0-09831fa17cef@imgtec.com

---
Brajesh Gupta (2):
      drm/imagination: Improve firmware power off for layout_mars config
      drm/imagination: Skip 2nd thread DM association for non META Firmware

 drivers/gpu/drm/imagination/pvr_fw_startstop.c | 100 ++++++++++++++++---------
 1 file changed, 66 insertions(+), 34 deletions(-)
---
base-commit: 68b271a3a94cfd6c7695a96b6398b52feb89e2c2
change-id: 20260304-b4-staging-layout_mars_base-dfbbf1a53b3c

Best regards,
-- 
Brajesh Gupta <brajesh.gupta@imgtec.com>

[PATCH v2 1/2] drm/imagination: Improve firmware power off for layout_mars config

[Brajesh Gupta]

In layout_mars HW config, Firmware MCU moved from Sidekick to new Mars
domain so Firmware takes care of powering down Sidekick/Jones and SLC.
Skip checks for those from kernel and check idle bits for Firmware MCU
and system arbiter excluding SOCIF.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_fw_startstop.c | 85 +++++++++++++++++---------
 1 file changed, 57 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_startstop.c b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
index dcbb9903e791..e47224ac0547 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_startstop.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
@@ -209,18 +209,32 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
 					 ROGUE_CR_SIDEKICK_IDLE_SOCIF_EN |
 					 ROGUE_CR_SIDEKICK_IDLE_HOSTIF_EN);
 	bool skip_garten_idle = false;
+	u64 layout_mars_value = 0;
+	bool layout_mars = false;
+	bool meta_fw = pvr_dev->fw_dev.processor_type == PVR_FW_PROCESSOR_TYPE_META;
 	u32 reg_value;
 	int err;
 
+	if (PVR_FEATURE_VALUE(pvr_dev, layout_mars, &layout_mars_value) == 0)
+		layout_mars = layout_mars_value > 0;
+
 	/*
-	 * Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper.
-	 * For cores with the LAYOUT_MARS feature, SIDEKICK would have been
+	 * For cores with the LAYOUT_MARS feature, SIDEKICK and SLC would have been
 	 * powered down by the FW.
 	 */
-	err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
-				sidekick_idle_mask, POLL_TIMEOUT_USEC);
-	if (err)
-		return err;
+	if (!layout_mars) {
+		/* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. */
+		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
+					sidekick_idle_mask, POLL_TIMEOUT_USEC);
+		if (err)
+			return err;
+
+		/* Wait for SLC to signal IDLE. */
+		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_MASKFULL,
+					 ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
+		if (err)
+			return err;
+	}
 
 	/* Unset MTS DM association with threads. */
 	pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC,
@@ -270,27 +284,25 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
 		return err;
 
 	/*
-	 * Wait for SLC to signal IDLE.
-	 * For cores with the LAYOUT_MARS feature, SLC would have been powered
-	 * down by the FW.
+	 * For cores with the LAYOUT_MARS feature, SIDEKICK and SLC would have been
+	 * powered down by the FW.
 	 */
-	err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE,
-				ROGUE_CR_SLC_IDLE_MASKFULL,
-				ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
-	if (err)
-		return err;
+	if (!layout_mars) {
+		/* Wait for SLC to signal IDLE. */
+		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE,
+					ROGUE_CR_SLC_IDLE_MASKFULL,
+					ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
+		if (err)
+			return err;
 
-	/*
-	 * Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper.
-	 * For cores with the LAYOUT_MARS feature, SIDEKICK would have been powered
-	 * down by the FW.
-	 */
-	err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
-				sidekick_idle_mask, POLL_TIMEOUT_USEC);
-	if (err)
-		return err;
+		/* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. */
+		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
+					sidekick_idle_mask, POLL_TIMEOUT_USEC);
+		if (err)
+			return err;
+	}
 
-	if (pvr_dev->fw_dev.processor_type == PVR_FW_PROCESSOR_TYPE_META) {
+	if (meta_fw) {
 		err = pvr_meta_cr_read32(pvr_dev, META_CR_TxVECINT_BHALT, &reg_value);
 		if (err)
 			return err;
@@ -304,11 +316,28 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
 			skip_garten_idle = true;
 	}
 
-	if (!skip_garten_idle) {
-		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
-					ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
-					ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
+	if (meta_fw || !layout_mars) {
+		if (!skip_garten_idle) {
+			err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
+						ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
+						ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
+						POLL_TIMEOUT_USEC);
+			if (err)
+				return err;
+		}
+	} else {
+		/*
+		 * As FW core has been moved from SIDEKICK to the new MARS domain, checking
+		 * idle bits for CPU & System Arbiter excluding SOCIF which will never be
+		 * idle if Host polling on this register
+		 */
+		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_MARS_IDLE,
+					ROGUE_CR_MARS_IDLE_CPU_EN |
+					ROGUE_CR_MARS_IDLE_MH_SYSARB0_EN,
+					ROGUE_CR_MARS_IDLE_CPU_EN |
+					ROGUE_CR_MARS_IDLE_MH_SYSARB0_EN,
 					POLL_TIMEOUT_USEC);
+
 		if (err)
 			return err;
 	}

-- 
2.43.0

Claude review: drm/imagination: Improve firmware power off for layout_mars config

[Claude Code Review Bot]

Patch Review

**Logic and correctness:**

The core logic change is correct — when `layout_mars` is true, Sidekick and SLC have been powered down by the firmware, so the kernel should not poll their idle registers. Instead, at the end, the MARS_IDLE register is polled for CPU and system arbiter (excluding SOCIF).

**Variable declaration ordering (minor style):**

```c
	u64 layout_mars_value = 0;
	bool layout_mars = false;
	bool meta_fw = pvr_dev->fw_dev.processor_type == PVR_FW_PROCESSOR_TYPE_META;
	u32 reg_value;
```

The kernel coding style prefers reverse-Christmas-tree ordering for variable declarations (longest line first). `meta_fw` is the longest declaration but appears after the shorter ones. This is a minor nit.

**Early SLC idle check addition:**

The original code had the SLC idle check only after the "Extra Idle checks" block (BIF_STATUS_MMU etc.). This patch adds an *additional* SLC idle check before the MTS DM association unset:

```c
	if (!layout_mars) {
		/* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. */
		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
					sidekick_idle_mask, POLL_TIMEOUT_USEC);
		if (err)
			return err;

		/* Wait for SLC to signal IDLE. */
		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_MASKFULL,
					 ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
		if (err)
			return err;
	}
```

The original code only checked Sidekick idle before MTS teardown, and checked SLC idle *after* the extra idle checks. Now both Sidekick and SLC are checked in *both* locations for non-MARS configurations. The commit message doesn't explain why an additional early SLC idle check is needed. This should be clarified — is it a pre-existing bug fix or a new requirement? It's a semantic change that goes beyond the stated goal of skipping checks for LAYOUT_MARS.

**Alignment issue:**

```c
		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_MASKFULL,
					 ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
```

There's a stray extra space before `ROGUE_CR_SLC_IDLE_MASKFULL` on the continuation line (one space too many for alignment). Compare with the second occurrence which aligns correctly with tabs only.

**Comment style (minor):**

```c
		/*
		 * As FW core has been moved from SIDEKICK to the new MARS domain, checking
		 * idle bits for CPU & System Arbiter excluding SOCIF which will never be
		 * idle if Host polling on this register
		 */
```

Missing period at end of comment. Kernel style expects properly punctuated multi-line comments.

**Garten idle condition:**

```c
	if (meta_fw || !layout_mars) {
		if (!skip_garten_idle) {
```

This is correct: for META firmware, always check the Garten wrapper idle via SIDEKICK_IDLE. For non-META, non-MARS, also check it. For non-META MARS, use the MARS_IDLE register instead. The logic is sound, but the `else` branch (MARS idle) is only reachable when `!meta_fw && layout_mars`, which is the RISC-V MARS case. A brief comment clarifying this would help readability.

**Blank line before error check:**

```c
					POLL_TIMEOUT_USEC);

		if (err)
			return err;
```

There's a blank line between the function call and the `if (err)` check in the MARS_IDLE polling block. This is inconsistent with every other poll+check pattern in the function which has no blank line.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 2/2] drm/imagination: Skip 2nd thread DM association for non META Firmware

[Brajesh Gupta]

Only a META firmware can have two threads.

Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_fw_startstop.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_startstop.c b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
index e47224ac0547..2f90bc1bf084 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_startstop.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
@@ -243,12 +243,15 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
 	pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC,
 		       ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_MASKFULL &
 		       ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_DM_ASSOC_CLRMSK);
-	pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC,
-		       ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_MASKFULL &
-		       ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
-	pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC,
-		       ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_MASKFULL &
-		       ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
+
+	if (meta_fw) {
+		pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC,
+			   ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_MASKFULL &
+			   ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
+		pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC,
+			   ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_MASKFULL &
+			   ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
+	}
 
 	/* Extra Idle checks. */
 	err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_BIF_STATUS_MMU, 0,

-- 
2.43.0

Claude review: drm/imagination: Skip 2nd thread DM association for non META Firmware

[Claude Code Review Bot]

Patch Review

**Logic:**

Correct. Only META firmware processors have two threads, so the Thread1 DM association writes should be skipped for RISC-V (and any other non-META) firmware. The `meta_fw` variable was already introduced in patch 1, so this uses it cleanly.

**Indentation issue:**

```c
	if (meta_fw) {
		pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC,
			   ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_MASKFULL &
			   ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
		pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC,
			   ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_MASKFULL &
			   ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
	}
```

The continuation lines for the `pvr_cr_write32` arguments are indented with spaces to align to column position, but they don't match the surrounding code's alignment style. Before this patch, these same lines used tab+spaces to align the arguments under the opening parenthesis. After the patch, the arguments are indented less (3 tabs + 3 spaces) instead of matching the opening paren position. Compare with the Thread0 writes just above which use proper alignment. This should be fixed for consistency.

**Overall:** Both patches are straightforward and correct in intent. The main actionable items are:
1. Explain (in commit message or comment) why the early SLC idle check was added for non-MARS configs in patch 1.
2. Fix the alignment/indentation issues in both patches.
3. Fix the missing period in the multi-line comment and the stray blank line in patch 1.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Improve power off sequence

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: Improve power off sequence
Author: Brajesh Gupta <brajesh.gupta@imgtec.com>
Patches: 3
Reviewed: 2026-03-14T07:22:39.772730

---

This is a 2-patch series for the `drm/imagination` (PowerVR) driver that improves the firmware power-off sequence for devices with the `LAYOUT_MARS` hardware configuration. In LAYOUT_MARS, the firmware MCU has moved from the Sidekick domain to a new Mars domain, so the kernel must skip Sidekick/SLC idle polling (the firmware handles those) and instead poll the MARS_IDLE register. The second patch skips the second-thread DM association write for non-META firmware processors (which only have one thread).

The changes are logically sound and well-structured. There are a few minor issues worth flagging.

---
Generated by Claude Code Patch Reviewer