[PATCH v2 0/5] Power Management for Raspberry Pi V3D GPU

[PATCH v2 0/5] Power Management for Raspberry Pi V3D GPU

[Maíra Canal]

This series introduces Runtime PM for Raspberry Pi's GPU, V3D.
Currently, the GPU clock stays up during the whole operation, even if
the GPU is idle. By introducing Runtime PM, we can now turn off the
clock completely during idle. For example, with this series, when
checking `vcgencmd measure_clock v3d` in the Raspberry Pi 5, we get:

(idle)

$ vcgencmd measure_clock v3d
frequency(0)=0

(running glmark2)

$ vcgencmd measure_clock v3d
frequency(0)=960016128

To implement PM for V3D, it was needed to add a prepare and unprepare
hook to RPi's firmware clocks. Currently, they don't turn on and off,
nor lower the clock rate. Therefore, PATCH 2/5 addresses this issue in
clk/bcm/clk-raspberrypi.c.

Apart from the first patch, the last three patches are related to PM
enablement in the V3D driver.

To ease testing in Raspberry Pi 4 and 5, I prepared a downstream branch
backporting this series to rpi-6.12.y [1].

[1] https://github.com/mairacanal/linux-rpi/tree/v3d/downstream/power-management-v2

Best Regards,
- Maíra

---
v1 -> v2: https://lore.kernel.org/r/20250728-v3d-power-management-v1-0-780f922b1048@igalia.com

- [1/5] NEW PATCH: "clk: bcm: rpi: Add missing logs if firmware fails" (Stefan Wahren)
- [2/5] Remove the "Fixes:" tag (Stefan Wahren)
- [2/5] dev_err_ratelimited() instead of dev_err() (Stefan Wahren)
- [2/5] Instead of logging the clock ID, use clk_hw_get_name(hw) to log the name (Stefan Wahren)
- [2/5] Add a newline character at the end of the log message (Stefan Wahren)
- [2/5] Use CLK_IS_CRITICAL for all clocks that can't be disabled (Maxime Ripard)
- [3/5] NEW PATCH: "clk: bcm: rpi: Maximize V3D clock"
- [4/5] Use devm_reset_control_get_optional_exclusive() (Philipp Zabel)
- [4/5] Make sure that resource are cleaned in the inverse order of allocation (Philipp Zabel)

---
Maíra Canal (4):
      clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing
      clk: bcm: rpi: Maximize V3D clock
      drm/v3d: Allocate all resources before enabling the clock
      drm/v3d: Introduce Runtime Power Management

Stefan Wahren (1):
      clk: bcm: rpi: Add missing logs if firmware fails

 drivers/clk/bcm/clk-raspberrypi.c |  72 ++++++++++++++++-
 drivers/gpu/drm/v3d/Makefile      |   3 +-
 drivers/gpu/drm/v3d/v3d_debugfs.c |  23 +++++-
 drivers/gpu/drm/v3d/v3d_drv.c     | 160 ++++++++++++++++++--------------------
 drivers/gpu/drm/v3d/v3d_drv.h     |  21 ++++-
 drivers/gpu/drm/v3d/v3d_gem.c     |  18 ++++-
 drivers/gpu/drm/v3d/v3d_irq.c     |  15 ++--
 drivers/gpu/drm/v3d/v3d_mmu.c     |  12 ++-
 drivers/gpu/drm/v3d/v3d_power.c   |  79 +++++++++++++++++++
 drivers/gpu/drm/v3d/v3d_submit.c  |  19 ++++-
 10 files changed, 311 insertions(+), 111 deletions(-)
---
base-commit: 518867b09394217d13f6e05f704450bd9d2c8eeb
change-id: 20250728-v3d-power-management-eebb2024dc96

[PATCH v2 1/5] clk: bcm: rpi: Add missing logs if firmware fails

[Maíra Canal]

From: Stefan Wahren <wahrenst@gmx.net>

In contrary to raspberrypi_fw_set_rate(), the ops for is_prepared() and
recalc_rate() silently ignore firmware errors by just returning 0.
Since these operations should never fail, add at least error logs
to inform the user.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/clk/bcm/clk-raspberrypi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 8e4fde03ed232b464165f524d27744b4ced93a60..166d0bec380310e8b98f91568efa4aa88401af4f 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -194,8 +194,11 @@ static int raspberrypi_fw_is_prepared(struct clk_hw *hw)
 
 	ret = raspberrypi_clock_property(rpi->firmware, data,
 					 RPI_FIRMWARE_GET_CLOCK_STATE, &val);
-	if (ret)
+	if (ret) {
+		dev_err_ratelimited(rpi->dev, "Failed to get %s state: %d\n",
+				    clk_hw_get_name(hw), ret);
 		return 0;
+	}
 
 	return !!(val & RPI_FIRMWARE_STATE_ENABLE_BIT);
 }
@@ -211,8 +214,11 @@ static unsigned long raspberrypi_fw_get_rate(struct clk_hw *hw,
 
 	ret = raspberrypi_clock_property(rpi->firmware, data,
 					 RPI_FIRMWARE_GET_CLOCK_RATE, &val);
-	if (ret)
+	if (ret) {
+		dev_err_ratelimited(rpi->dev, "Failed to get %s frequency: %d\n",
+				    clk_hw_get_name(hw), ret);
 		return 0;
+	}
 
 	return val;
 }

-- 
2.50.0

[PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maíra Canal]

Currently, when we prepare or unprepare RPi's clocks, we don't actually
enable/disable the firmware clock. This means that
`clk_disable_unprepare()` doesn't actually change the clock state at
all, nor does it lowers the clock rate.

From the Mailbox Property Interface documentation [1], we can see that
we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
prepare and an unprepare hook for RPi's firmware clock.

As now the clocks are actually turned off, some of them are now marked
CLK_IS_CRITICAL, as those are required to be on during the whole system
operation.

Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
Signed-off-by: Maíra Canal <mcanal@igalia.com>

---

About the pixel clock: currently, if we actually disable the pixel
clock during a hotplug, the system will crash. This happens in the
RPi 4.

The crash happens after we disabled the CRTC (thus, the pixel clock),
but before the end of atomic commit tail. As vc4's pixel valve doesn't
directly hold a reference to its clock – we use the HDMI encoder to
manage the pixel clock – I believe we might be disabling the clock
before we should.

After this investigation, I decided to keep things as they current are:
the pixel clock is never disabled, as fixing it would go out of
the scope of this series.
---
 drivers/clk/bcm/clk-raspberrypi.c | 56 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 166d0bec380310e8b98f91568efa4aa88401af4f..70acfa68827d84670c645bedd17bf0e181aadfbb 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -68,6 +68,7 @@ struct raspberrypi_clk_variant {
 	char		*clkdev;
 	unsigned long	min_rate;
 	bool		minimize;
+	u32		flags;
 };
 
 static struct raspberrypi_clk_variant
@@ -75,6 +76,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	[RPI_FIRMWARE_ARM_CLK_ID] = {
 		.export = true,
 		.clkdev = "cpu0",
+		.flags = CLK_IS_CRITICAL,
 	},
 	[RPI_FIRMWARE_CORE_CLK_ID] = {
 		.export = true,
@@ -90,6 +92,12 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 		 * always use the minimum the drivers will let us.
 		 */
 		.minimize = true,
+
+		/*
+		 * It should never be disabled as it drives the bus for
+		 * everything else.
+		 */
+		.flags = CLK_IS_CRITICAL,
 	},
 	[RPI_FIRMWARE_M2MC_CLK_ID] = {
 		.export = true,
@@ -115,6 +123,15 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 		 * drivers will let us.
 		 */
 		.minimize = true,
+
+		/*
+		 * As mentioned above, this clock is disabled during boot,
+		 * the firmware will skip the HSM initialization, resulting
+		 * in a bus lockup. Therefore, make sure it's enabled
+		 * during boot, but after it, it can be enabled/disabled
+		 * by the driver.
+		 */
+		.flags = CLK_IGNORE_UNUSED,
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
@@ -123,10 +140,12 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	[RPI_FIRMWARE_PIXEL_CLK_ID] = {
 		.export = true,
 		.minimize = true,
+		.flags = CLK_IS_CRITICAL,
 	},
 	[RPI_FIRMWARE_HEVC_CLK_ID] = {
 		.export = true,
 		.minimize = true,
+		.flags = CLK_IS_CRITICAL,
 	},
 	[RPI_FIRMWARE_ISP_CLK_ID] = {
 		.export = true,
@@ -135,6 +154,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	[RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = {
 		.export = true,
 		.minimize = true,
+		.flags = CLK_IS_CRITICAL,
 	},
 	[RPI_FIRMWARE_VEC_CLK_ID] = {
 		.export = true,
@@ -265,7 +285,41 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static int raspberrypi_fw_prepare(struct clk_hw *hw)
+{
+	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
+	struct raspberrypi_clk *rpi = data->rpi;
+	u32 state = RPI_FIRMWARE_STATE_ENABLE_BIT;
+	int ret;
+
+	ret = raspberrypi_clock_property(rpi->firmware, data,
+					 RPI_FIRMWARE_SET_CLOCK_STATE, &state);
+	if (ret)
+		dev_err_ratelimited(rpi->dev,
+				    "Failed to set clock %s state to on: %d\n",
+				    clk_hw_get_name(hw), ret);
+
+	return ret;
+}
+
+static void raspberrypi_fw_unprepare(struct clk_hw *hw)
+{
+	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
+	struct raspberrypi_clk *rpi = data->rpi;
+	u32 state = 0;
+	int ret;
+
+	ret = raspberrypi_clock_property(rpi->firmware, data,
+					 RPI_FIRMWARE_SET_CLOCK_STATE, &state);
+	if (ret)
+		dev_err_ratelimited(rpi->dev,
+				    "Failed to set clock %s state to off: %d\n",
+				    clk_hw_get_name(hw), ret);
+}
+
 static const struct clk_ops raspberrypi_firmware_clk_ops = {
+	.prepare        = raspberrypi_fw_prepare,
+	.unprepare      = raspberrypi_fw_unprepare,
 	.is_prepared	= raspberrypi_fw_is_prepared,
 	.recalc_rate	= raspberrypi_fw_get_rate,
 	.determine_rate	= raspberrypi_fw_dumb_determine_rate,
@@ -295,7 +349,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 	if (!init.name)
 		return ERR_PTR(-ENOMEM);
 	init.ops = &raspberrypi_firmware_clk_ops;
-	init.flags = CLK_GET_RATE_NOCACHE;
+	init.flags = variant->flags | CLK_GET_RATE_NOCACHE;
 
 	data->hw.init = &init;
 

-- 
2.50.0

[PATCH v2 3/5] clk: bcm: rpi: Maximize V3D clock

[Maíra Canal]

Although minimizing the clock rate is the best for most scenarios, as
stated in commit 4d85abb0fb8e ("clk: bcm: rpi: Enable minimize for all
firmware clocks"), when it comes to the GPU, it's ideal to have the
maximum rate allowed.

Add an option to maximize a firmware clock's rate when the clock is
enabled and set this option for V3D.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/clk/bcm/clk-raspberrypi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 70acfa68827d84670c645bedd17bf0e181aadfbb..1a9162f0ae31e330c46f6eafdd00350599b0eede 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -68,6 +68,7 @@ struct raspberrypi_clk_variant {
 	char		*clkdev;
 	unsigned long	min_rate;
 	bool		minimize;
+	bool		maximize;
 	u32		flags;
 };
 
@@ -135,7 +136,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
-		.minimize = true,
+		.maximize = true,
 	},
 	[RPI_FIRMWARE_PIXEL_CLK_ID] = {
 		.export = true,
@@ -386,6 +387,9 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
 		}
 	}
 
+	if (variant->maximize)
+		variant->min_rate = max_rate;
+
 	if (variant->min_rate) {
 		unsigned long rate;
 

-- 
2.50.0

[PATCH v2 4/5] drm/v3d: Allocate all resources before enabling the clock

[Maíra Canal]

Move all resource allocation operations before actually enabling the
clock, as those operation don't require the GPU to be powered on.

While here, use devm_reset_control_get_optional_exclusive() instead of
open-code it.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.c | 93 +++++++++++++++++++++----------------------
 drivers/gpu/drm/v3d/v3d_drv.h |  3 +-
 drivers/gpu/drm/v3d/v3d_gem.c | 14 +++++--
 drivers/gpu/drm/v3d/v3d_irq.c | 15 +++----
 4 files changed, 64 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 2def155ce496ec5f159f6bda9929aeaae141d1a6..4e4217c7052210262c7af431de5fc24e13ab6bd0 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -347,14 +347,50 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 			return ret;
 	}
 
+	if (v3d->ver < V3D_GEN_41) {
+		ret = map_regs(v3d, &v3d->gca_regs, "gca");
+		if (ret)
+			return ret;
+	}
+
+	v3d->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(v3d->reset))
+		return dev_err_probe(dev, PTR_ERR(v3d->reset),
+				     "Failed to get reset control\n");
+
+	if (!v3d->reset) {
+		ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
+		if (ret) {
+			dev_err(dev, "Failed to get bridge registers\n");
+			return ret;
+		}
+	}
+
 	v3d->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(v3d->clk))
 		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
 
+	ret = v3d_irq_init(v3d);
+	if (ret)
+		return ret;
+
+	v3d_perfmon_init(v3d);
+
+	v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr,
+					GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
+	if (!v3d->mmu_scratch) {
+		dev_err(dev, "Failed to allocate MMU scratch page\n");
+		return -ENOMEM;
+	}
+
+	ret = v3d_gem_allocate(drm);
+	if (ret)
+		goto dma_free;
+
 	ret = clk_prepare_enable(v3d->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't enable the V3D clock\n");
-		return ret;
+		goto gem_destroy;
 	}
 
 	v3d_idle_sms(v3d);
@@ -381,45 +417,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	ident3 = V3D_READ(V3D_HUB_IDENT3);
 	v3d->rev = V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPREV);
 
-	v3d_perfmon_init(v3d);
-
-	v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
-	if (IS_ERR(v3d->reset)) {
-		ret = PTR_ERR(v3d->reset);
-
-		if (ret == -EPROBE_DEFER)
-			goto clk_disable;
-
-		v3d->reset = NULL;
-		ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
-		if (ret) {
-			dev_err(dev,
-				"Failed to get reset control or bridge regs\n");
-			goto clk_disable;
-		}
-	}
-
-	if (v3d->ver < V3D_GEN_41) {
-		ret = map_regs(v3d, &v3d->gca_regs, "gca");
-		if (ret)
-			goto clk_disable;
-	}
-
-	v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr,
-					GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
-	if (!v3d->mmu_scratch) {
-		dev_err(dev, "Failed to allocate MMU scratch page\n");
-		ret = -ENOMEM;
-		goto clk_disable;
-	}
-
-	ret = v3d_gem_init(drm);
-	if (ret)
-		goto dma_free;
-
-	ret = v3d_irq_init(v3d);
-	if (ret)
-		goto gem_destroy;
+	v3d_gem_init(drm);
+	v3d_irq_enable(v3d);
 
 	ret = drm_dev_register(drm, 0);
 	if (ret)
@@ -435,12 +434,13 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	drm_dev_unregister(drm);
 irq_disable:
 	v3d_irq_disable(v3d);
+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;
 }
 
@@ -454,14 +454,13 @@ static void v3d_platform_drm_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(drm);
 
-	v3d_gem_destroy(drm);
-
-	dma_free_wc(v3d->drm.dev, 4096, v3d->mmu_scratch,
-		    v3d->mmu_scratch_paddr);
-
 	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);
 }
 
 static struct platform_driver v3d_platform_driver = {
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index dabda7aaf00074d8de42dcdb345d5f3331ac13b2..aa33dcdc6a371393576dd8c20ab1dae920039a0c 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -565,7 +565,8 @@ extern const struct dma_fence_ops v3d_fence_ops;
 struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue queue);
 
 /* v3d_gem.c */
-int v3d_gem_init(struct drm_device *dev);
+int v3d_gem_allocate(struct drm_device *dev);
+void v3d_gem_init(struct drm_device *dev);
 void v3d_gem_destroy(struct drm_device *dev);
 void v3d_reset_sms(struct v3d_dev *v3d);
 void v3d_reset(struct v3d_dev *v3d);
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index d7d16da78db328f004d1d702731d1a1b5437a394..4626ee6e4ac4412c293a87e8a8cc5ec84376cf24 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -257,7 +257,7 @@ v3d_invalidate_caches(struct v3d_dev *v3d)
 }
 
 int
-v3d_gem_init(struct drm_device *dev)
+v3d_gem_allocate(struct drm_device *dev)
 {
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	u32 pt_size = 4096 * 1024;
@@ -302,9 +302,6 @@ v3d_gem_init(struct drm_device *dev)
 		return -ENOMEM;
 	}
 
-	v3d_init_hw_state(v3d);
-	v3d_mmu_set_page_table(v3d);
-
 	v3d_gemfs_init(v3d);
 
 	ret = v3d_sched_init(v3d);
@@ -318,6 +315,15 @@ v3d_gem_init(struct drm_device *dev)
 	return 0;
 }
 
+void
+v3d_gem_init(struct drm_device *dev)
+{
+	struct v3d_dev *v3d = to_v3d_dev(dev);
+
+	v3d_init_hw_state(v3d);
+	v3d_mmu_set_page_table(v3d);
+}
+
 void
 v3d_gem_destroy(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 2cca5d3a26a22cf03d2c4e9ae0688ab57e0eedcd..89eb3034db4bddf878a88f17793b2ea08f4c37d5 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -260,17 +260,10 @@ v3d_hub_irq(int irq, void *arg)
 int
 v3d_irq_init(struct v3d_dev *v3d)
 {
-	int irq1, ret, core;
+	int irq1, ret;
 
 	INIT_WORK(&v3d->overflow_mem_work, v3d_overflow_mem_work);
 
-	/* 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));
-
 	irq1 = platform_get_irq_optional(v3d_to_pdev(v3d), 1);
 	if (irq1 == -EPROBE_DEFER)
 		return irq1;
@@ -297,7 +290,6 @@ v3d_irq_init(struct v3d_dev *v3d)
 			goto fail;
 	}
 
-	v3d_irq_enable(v3d);
 	return 0;
 
 fail:
@@ -311,6 +303,11 @@ v3d_irq_enable(struct v3d_dev *v3d)
 {
 	int core;
 
+	/* 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));
+
 	/* Enable our set of interrupts, masking out any others. */
 	for (core = 0; core < v3d->cores; core++) {
 		V3D_CORE_WRITE(core, V3D_CTL_INT_MSK_SET, ~V3D_CORE_IRQS(v3d->ver));

-- 
2.50.0

[PATCH v2 5/5] drm/v3d: Introduce Runtime Power Management

[Maíra Canal]

Commit 90a64adb0876 ("drm/v3d: Get rid of pm code") removed the last
bits of power management code that V3D had, which were actually never
hooked. Therefore, currently, the GPU clock is enabled during probe
and only disabled when removing the driver.

Implement proper power management using the kernel's Runtime PM
framework.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/Makefile      |  3 +-
 drivers/gpu/drm/v3d/v3d_debugfs.c | 23 ++++++++++-
 drivers/gpu/drm/v3d/v3d_drv.c     | 83 ++++++++++++++++++---------------------
 drivers/gpu/drm/v3d/v3d_drv.h     | 18 +++++++++
 drivers/gpu/drm/v3d/v3d_gem.c     |  6 ++-
 drivers/gpu/drm/v3d/v3d_mmu.c     | 12 +++++-
 drivers/gpu/drm/v3d/v3d_power.c   | 79 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/v3d/v3d_submit.c  | 19 +++++++--
 8 files changed, 188 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/v3d/Makefile b/drivers/gpu/drm/v3d/Makefile
index fcf710926057b34701792e1ee0458ae8ca5b25e3..c2d6d4b953435307f21c365e0ede1c2927407e06 100644
--- a/drivers/gpu/drm/v3d/Makefile
+++ b/drivers/gpu/drm/v3d/Makefile
@@ -14,7 +14,8 @@ v3d-y := \
 	v3d_sched.o \
 	v3d_sysfs.o \
 	v3d_submit.o \
-	v3d_gemfs.o
+	v3d_gemfs.o \
+	v3d_power.o
 
 v3d-$(CONFIG_DEBUG_FS) += v3d_debugfs.o
 
diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c b/drivers/gpu/drm/v3d/v3d_debugfs.c
index 7e789e181af0ac138044f194a29555c30ab01836..d4cd4360ad21a22ebd0d9df7a93427bdb75d2c9c 100644
--- a/drivers/gpu/drm/v3d/v3d_debugfs.c
+++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
@@ -96,7 +96,11 @@ static int v3d_v3d_debugfs_regs(struct seq_file *m, void *unused)
 	struct drm_debugfs_entry *entry = m->private;
 	struct drm_device *dev = entry->dev;
 	struct v3d_dev *v3d = to_v3d_dev(dev);
-	int i, core;
+	int i, core, ret;
+
+	ret = v3d_pm_runtime_get(v3d);
+	if (ret)
+		return ret;
 
 	for (i = 0; i < ARRAY_SIZE(v3d_hub_reg_defs); i++) {
 		const struct v3d_reg_def *def = &v3d_hub_reg_defs[i];
@@ -138,6 +142,8 @@ static int v3d_v3d_debugfs_regs(struct seq_file *m, void *unused)
 		}
 	}
 
+	v3d_pm_runtime_put(v3d);
+
 	return 0;
 }
 
@@ -147,7 +153,11 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void *unused)
 	struct drm_device *dev = entry->dev;
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 	u32 ident0, ident1, ident2, ident3, cores;
-	int core;
+	int core, ret;
+
+	ret = v3d_pm_runtime_get(v3d);
+	if (ret)
+		return ret;
 
 	ident0 = V3D_READ(V3D_HUB_IDENT0);
 	ident1 = V3D_READ(V3D_HUB_IDENT1);
@@ -206,6 +216,8 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void *unused)
 		}
 	}
 
+	v3d_pm_runtime_put(v3d);
+
 	return 0;
 }
 
@@ -233,6 +245,11 @@ static int v3d_measure_clock(struct seq_file *m, void *unused)
 	uint32_t cycles;
 	int core = 0;
 	int measure_ms = 1000;
+	int ret;
+
+	ret = v3d_pm_runtime_get(v3d);
+	if (ret)
+		return ret;
 
 	if (v3d->ver >= V3D_GEN_41) {
 		int cycle_count_reg = V3D_PCTR_CYCLE_COUNT(v3d->ver);
@@ -252,6 +269,8 @@ static int v3d_measure_clock(struct seq_file *m, void *unused)
 	msleep(measure_ms);
 	cycles = V3D_CORE_READ(core, V3D_PCTR_0_PCTR0);
 
+	v3d_pm_runtime_put(v3d);
+
 	seq_printf(m, "cycles: %d (%d.%d Mhz)\n",
 		   cycles,
 		   cycles / (measure_ms * 1000),
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 4e4217c7052210262c7af431de5fc24e13ab6bd0..6e4a9281e741e51ea3841fc4241bd7e534b70c19 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -58,6 +58,7 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data,
 		[DRM_V3D_PARAM_V3D_CORE0_IDENT1] = V3D_CTL_IDENT1,
 		[DRM_V3D_PARAM_V3D_CORE0_IDENT2] = V3D_CTL_IDENT2,
 	};
+	int ret;
 
 	if (args->pad != 0)
 		return -EINVAL;
@@ -74,12 +75,19 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data,
 		if (args->value != 0)
 			return -EINVAL;
 
+		ret = v3d_pm_runtime_get(v3d);
+		if (ret)
+			return ret;
+
 		if (args->param >= DRM_V3D_PARAM_V3D_CORE0_IDENT0 &&
 		    args->param <= DRM_V3D_PARAM_V3D_CORE0_IDENT2) {
 			args->value = V3D_CORE_READ(0, offset);
 		} else {
 			args->value = V3D_READ(offset);
 		}
+
+		v3d_pm_runtime_put(v3d);
+
 		return 0;
 	}
 
@@ -274,36 +282,6 @@ static const struct of_device_id v3d_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, v3d_of_match);
 
-static void
-v3d_idle_sms(struct v3d_dev *v3d)
-{
-	if (v3d->ver < V3D_GEN_71)
-		return;
-
-	V3D_SMS_WRITE(V3D_SMS_TEE_CS, V3D_SMS_CLEAR_POWER_OFF);
-
-	if (wait_for((V3D_GET_FIELD(V3D_SMS_READ(V3D_SMS_TEE_CS),
-				    V3D_SMS_STATE) == V3D_SMS_IDLE), 100)) {
-		DRM_ERROR("Failed to power up SMS\n");
-	}
-
-	v3d_reset_sms(v3d);
-}
-
-static void
-v3d_power_off_sms(struct v3d_dev *v3d)
-{
-	if (v3d->ver < V3D_GEN_71)
-		return;
-
-	V3D_SMS_WRITE(V3D_SMS_TEE_CS, V3D_SMS_POWER_OFF);
-
-	if (wait_for((V3D_GET_FIELD(V3D_SMS_READ(V3D_SMS_TEE_CS),
-				    V3D_SMS_STATE) == V3D_SMS_POWER_OFF_STATE), 100)) {
-		DRM_ERROR("Failed to power off SMS\n");
-	}
-}
-
 static int
 map_regs(struct v3d_dev *v3d, void __iomem **regs, const char *name)
 {
@@ -387,19 +365,26 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	if (ret)
 		goto dma_free;
 
-	ret = clk_prepare_enable(v3d->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Couldn't enable the V3D clock\n");
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
 		goto gem_destroy;
-	}
 
-	v3d_idle_sms(v3d);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		goto gem_destroy;
+
+	/* If PM is disabled, we need to call v3d_power_resume() manually. */
+	if (!IS_ENABLED(CONFIG_PM)) {
+		ret = v3d_power_resume(dev);
+		if (ret)
+			goto gem_destroy;
+	}
 
 	mmu_debug = V3D_READ(V3D_MMU_DEBUG_INFO);
 	mask = DMA_BIT_MASK(30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_PA_WIDTH));
 	ret = dma_set_mask_and_coherent(dev, mask);
 	if (ret)
-		goto clk_disable;
+		goto runtime_pm_put;
 
 	v3d->va_width = 30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_VA_WIDTH);
 
@@ -418,25 +403,27 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d->rev = V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPREV);
 
 	v3d_gem_init(drm);
-	v3d_irq_enable(v3d);
+
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
 
 	ret = drm_dev_register(drm, 0);
 	if (ret)
-		goto irq_disable;
+		goto runtime_pm_put;
 
 	ret = v3d_sysfs_init(dev);
 	if (ret)
 		goto drm_unregister;
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
 drm_unregister:
 	drm_dev_unregister(drm);
-irq_disable:
-	v3d_irq_disable(v3d);
-clk_disable:
-	v3d_power_off_sms(v3d);
-	clk_disable_unprepare(v3d->clk);
+runtime_pm_put:
+	pm_runtime_put_sync_suspend(dev);
 gem_destroy:
 	v3d_gem_destroy(drm);
 dma_free:
@@ -454,21 +441,27 @@ static void v3d_platform_drm_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(drm);
 
-	v3d_power_off_sms(v3d);
+	pm_runtime_suspend(dev);
 
-	clk_disable_unprepare(v3d->clk);
+	/* If PM is disabled, we need to call v3d_power_suspend() manually. */
+	if (!IS_ENABLED(CONFIG_PM))
+		v3d_power_suspend(dev);
 
 	v3d_gem_destroy(drm);
 
 	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
 }
 
+static DEFINE_RUNTIME_DEV_PM_OPS(v3d_pm_ops, v3d_power_suspend,
+				 v3d_power_resume, NULL);
+
 static struct platform_driver v3d_platform_driver = {
 	.probe		= v3d_platform_drm_probe,
 	.remove		= v3d_platform_drm_remove,
 	.driver		= {
 		.name	= "v3d",
 		.of_match_table = v3d_of_match,
+		.pm = pm_ptr(&v3d_pm_ops),
 	},
 };
 
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index aa33dcdc6a371393576dd8c20ab1dae920039a0c..c92b5fa6066be88b8bf04ff4dfd273c5e8cd441d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -3,6 +3,7 @@
 
 #include <linux/delay.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/spinlock_types.h>
 #include <linux/workqueue.h>
 
@@ -325,6 +326,8 @@ struct v3d_job {
 
 	/* Callback for the freeing of the job on refcount going to 0. */
 	void (*free)(struct kref *ref);
+
+	bool has_pm_ref;
 };
 
 struct v3d_bin_job {
@@ -602,6 +605,21 @@ int v3d_mmu_set_page_table(struct v3d_dev *v3d);
 void v3d_mmu_insert_ptes(struct v3d_bo *bo);
 void v3d_mmu_remove_ptes(struct v3d_bo *bo);
 
+/* v3d_power.c */
+int v3d_power_suspend(struct device *dev);
+int v3d_power_resume(struct device *dev);
+
+static __always_inline int v3d_pm_runtime_get(struct v3d_dev *v3d)
+{
+	return pm_runtime_resume_and_get(v3d->drm.dev);
+}
+
+static __always_inline int v3d_pm_runtime_put(struct v3d_dev *v3d)
+{
+	pm_runtime_mark_last_busy(v3d->drm.dev);
+	return pm_runtime_put_autosuspend(v3d->drm.dev);
+}
+
 /* v3d_sched.c */
 void v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info,
 				   unsigned int count);
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 4626ee6e4ac4412c293a87e8a8cc5ec84376cf24..fd2582629a431c6e41d07a9edb3608190ee23e5e 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -128,6 +128,9 @@ v3d_reset(struct v3d_dev *v3d)
 	DRM_DEV_ERROR(dev->dev, "Resetting GPU for hang.\n");
 	DRM_DEV_ERROR(dev->dev, "V3D_ERR_STAT: 0x%08x\n",
 		      V3D_CORE_READ(0, V3D_ERR_STAT));
+
+	v3d_pm_runtime_get(v3d);
+
 	trace_v3d_reset_begin(dev);
 
 	/* XXX: only needed for safe powerdown, not reset. */
@@ -144,6 +147,8 @@ v3d_reset(struct v3d_dev *v3d)
 	v3d_perfmon_stop(v3d, v3d->active_perfmon, false);
 
 	trace_v3d_reset_end(dev);
+
+	v3d_pm_runtime_put(v3d);
 }
 
 static void
@@ -321,7 +326,6 @@ v3d_gem_init(struct drm_device *dev)
 	struct v3d_dev *v3d = to_v3d_dev(dev);
 
 	v3d_init_hw_state(v3d);
-	v3d_mmu_set_page_table(v3d);
 }
 
 void
diff --git a/drivers/gpu/drm/v3d/v3d_mmu.c b/drivers/gpu/drm/v3d/v3d_mmu.c
index a25d25a8ae617bf68e133e1793cd0bb930bb07f6..1699819756aadfc40f7d41ff19847d42ddf10dce 100644
--- a/drivers/gpu/drm/v3d/v3d_mmu.c
+++ b/drivers/gpu/drm/v3d/v3d_mmu.c
@@ -37,7 +37,13 @@ static bool v3d_mmu_is_aligned(u32 page, u32 page_address, size_t alignment)
 
 int v3d_mmu_flush_all(struct v3d_dev *v3d)
 {
-	int ret;
+	int ret = 0;
+
+	pm_runtime_get_noresume(v3d->drm.dev);
+
+	/* Flush the PTs only if we're already awake */
+	if (!pm_runtime_active(v3d->drm.dev))
+		goto pm_put;
 
 	V3D_WRITE(V3D_MMUC_CONTROL, V3D_MMUC_CONTROL_FLUSH |
 		  V3D_MMUC_CONTROL_ENABLE);
@@ -46,7 +52,7 @@ int v3d_mmu_flush_all(struct v3d_dev *v3d)
 			 V3D_MMUC_CONTROL_FLUSHING), 100);
 	if (ret) {
 		dev_err(v3d->drm.dev, "MMUC flush wait idle failed\n");
-		return ret;
+		goto pm_put;
 	}
 
 	V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL) |
@@ -57,6 +63,8 @@ int v3d_mmu_flush_all(struct v3d_dev *v3d)
 	if (ret)
 		dev_err(v3d->drm.dev, "MMU TLB clear wait idle failed\n");
 
+pm_put:
+	pm_runtime_put_autosuspend(v3d->drm.dev);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/v3d/v3d_power.c b/drivers/gpu/drm/v3d/v3d_power.c
new file mode 100644
index 0000000000000000000000000000000000000000..33eecfd9825af90ee607fa2eae67666ba020ad27
--- /dev/null
+++ b/drivers/gpu/drm/v3d/v3d_power.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (C) 2025 Raspberry Pi */
+
+#include <linux/clk.h>
+#include <linux/reset.h>
+
+#include "v3d_drv.h"
+#include "v3d_regs.h"
+
+static void
+v3d_resume_sms(struct v3d_dev *v3d)
+{
+	if (v3d->ver < V3D_GEN_71)
+		return;
+
+	V3D_SMS_WRITE(V3D_SMS_TEE_CS, V3D_SMS_CLEAR_POWER_OFF);
+
+	if (wait_for((V3D_GET_FIELD(V3D_SMS_READ(V3D_SMS_TEE_CS),
+				    V3D_SMS_STATE) == V3D_SMS_IDLE), 100)) {
+		DRM_ERROR("Failed to power up SMS\n");
+	}
+
+	v3d_reset_sms(v3d);
+}
+
+static void
+v3d_suspend_sms(struct v3d_dev *v3d)
+{
+	if (v3d->ver < V3D_GEN_71)
+		return;
+
+	V3D_SMS_WRITE(V3D_SMS_TEE_CS, V3D_SMS_POWER_OFF);
+
+	if (wait_for((V3D_GET_FIELD(V3D_SMS_READ(V3D_SMS_TEE_CS),
+				    V3D_SMS_STATE) == V3D_SMS_POWER_OFF_STATE), 100)) {
+		DRM_ERROR("Failed to power off SMS\n");
+	}
+}
+
+int v3d_power_suspend(struct device *dev)
+{
+	struct v3d_dev *v3d = dev_get_drvdata(dev);
+
+	v3d_irq_disable(v3d);
+	v3d_suspend_sms(v3d);
+
+	if (v3d->reset)
+		reset_control_assert(v3d->reset);
+
+	clk_disable_unprepare(v3d->clk);
+
+	return 0;
+}
+
+int v3d_power_resume(struct device *dev)
+{
+	struct v3d_dev *v3d = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(v3d->clk);
+	if (ret)
+		return ret;
+
+	if (v3d->reset) {
+		ret = reset_control_deassert(v3d->reset);
+		if (ret)
+			goto clk_disable;
+	}
+
+	v3d_resume_sms(v3d);
+	v3d_mmu_set_page_table(v3d);
+	v3d_irq_enable(v3d);
+
+	return 0;
+
+clk_disable:
+	clk_disable_unprepare(v3d->clk);
+	return ret;
+}
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 5171ffe9012d4d0140d82d40af71ecbaf029a24a..5236d647608d4ecfc8b06d3163735f6ede5cac9f 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -102,6 +102,9 @@ v3d_job_free(struct kref *ref)
 	if (job->perfmon)
 		v3d_perfmon_put(job->perfmon);
 
+	if (job->has_pm_ref)
+		v3d_pm_runtime_put(job->v3d);
+
 	kfree(job);
 }
 
@@ -183,13 +186,13 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 				if (copy_from_user(&in, handle++, sizeof(in))) {
 					ret = -EFAULT;
 					DRM_DEBUG("Failed to copy wait dep handle.\n");
-					goto fail_deps;
+					goto fail_job_init;
 				}
 				ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv, in.handle, 0);
 
 				// TODO: Investigate why this was filtered out for the IOCTL.
 				if (ret && ret != -ENOENT)
-					goto fail_deps;
+					goto fail_job_init;
 			}
 		}
 	} else {
@@ -197,14 +200,22 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
 
 		// TODO: Investigate why this was filtered out for the IOCTL.
 		if (ret && ret != -ENOENT)
-			goto fail_deps;
+			goto fail_job_init;
+	}
+
+	/* CPU jobs don't require hardware resources */
+	if (queue != V3D_CPU) {
+		ret = v3d_pm_runtime_get(v3d);
+		if (ret)
+			goto fail_job_init;
+		job->has_pm_ref = true;
 	}
 
 	kref_init(&job->refcount);
 
 	return 0;
 
-fail_deps:
+fail_job_init:
 	drm_sched_job_cleanup(&job->base);
 	return ret;
 }

-- 
2.50.0

Re: [PATCH v2 4/5] drm/v3d: Allocate all resources before enabling the clock

[Philipp Zabel]

On Do, 2025-07-31 at 18:06 -0300, Maíra Canal wrote:
> Move all resource allocation operations before actually enabling the
> clock, as those operation don't require the GPU to be powered on.
> 
> While here, use devm_reset_control_get_optional_exclusive() instead of
> open-code it.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Stefan Wahren]

Hello Maíra,

Am 31.07.25 um 23:06 schrieb Maíra Canal:
> Currently, when we prepare or unprepare RPi's clocks, we don't actually
> enable/disable the firmware clock. This means that
> `clk_disable_unprepare()` doesn't actually change the clock state at
> all, nor does it lowers the clock rate.
>
>  From the Mailbox Property Interface documentation [1], we can see that
> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
> prepare and an unprepare hook for RPi's firmware clock.
>
> As now the clocks are actually turned off, some of them are now marked
> CLK_IS_CRITICAL, as those are required to be on during the whole system
> operation.
>
> Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
sorry this late reply, but I was in the holidays.

In general the approach looks good to me. Very old vc4 firmware versions 
doesn't support RPI_FIRMWARE_SET_CLOCK_STATE. I mention this because 
sometimes the setups on kernelci.org are not always up to date. But I 
think we should be save in this case.

Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
>
> ---
>
> About the pixel clock: currently, if we actually disable the pixel
> clock during a hotplug, the system will crash. This happens in the
> RPi 4.
>
> The crash happens after we disabled the CRTC (thus, the pixel clock),
> but before the end of atomic commit tail. As vc4's pixel valve doesn't
> directly hold a reference to its clock – we use the HDMI encoder to
> manage the pixel clock – I believe we might be disabling the clock
> before we should.
>
> After this investigation, I decided to keep things as they current are:
> the pixel clock is never disabled, as fixing it would go out of
> the scope of this series.
> ---
>   drivers/clk/bcm/clk-raspberrypi.c | 56 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index 166d0bec380310e8b98f91568efa4aa88401af4f..70acfa68827d84670c645bedd17bf0e181aadfbb 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -68,6 +68,7 @@ struct raspberrypi_clk_variant {
>   	char		*clkdev;
>   	unsigned long	min_rate;
>   	bool		minimize;
> +	u32		flags;
>   };
>   
>   static struct raspberrypi_clk_variant
> @@ -75,6 +76,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   	[RPI_FIRMWARE_ARM_CLK_ID] = {
>   		.export = true,
>   		.clkdev = "cpu0",
> +		.flags = CLK_IS_CRITICAL,
>   	},
>   	[RPI_FIRMWARE_CORE_CLK_ID] = {
>   		.export = true,
> @@ -90,6 +92,12 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   		 * always use the minimum the drivers will let us.
>   		 */
>   		.minimize = true,
> +
> +		/*
> +		 * It should never be disabled as it drives the bus for
> +		 * everything else.
> +		 */
> +		.flags = CLK_IS_CRITICAL,
>   	},
>   	[RPI_FIRMWARE_M2MC_CLK_ID] = {
>   		.export = true,
> @@ -115,6 +123,15 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   		 * drivers will let us.
>   		 */
>   		.minimize = true,
> +
> +		/*
> +		 * As mentioned above, this clock is disabled during boot,
> +		 * the firmware will skip the HSM initialization, resulting
> +		 * in a bus lockup. Therefore, make sure it's enabled
> +		 * during boot, but after it, it can be enabled/disabled
> +		 * by the driver.
> +		 */
> +		.flags = CLK_IGNORE_UNUSED,
>   	},
>   	[RPI_FIRMWARE_V3D_CLK_ID] = {
>   		.export = true,
> @@ -123,10 +140,12 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   	[RPI_FIRMWARE_PIXEL_CLK_ID] = {
>   		.export = true,
>   		.minimize = true,
> +		.flags = CLK_IS_CRITICAL,
>   	},
>   	[RPI_FIRMWARE_HEVC_CLK_ID] = {
>   		.export = true,
>   		.minimize = true,
> +		.flags = CLK_IS_CRITICAL,
>   	},
>   	[RPI_FIRMWARE_ISP_CLK_ID] = {
>   		.export = true,
> @@ -135,6 +154,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   	[RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = {
>   		.export = true,
>   		.minimize = true,
> +		.flags = CLK_IS_CRITICAL,
>   	},
>   	[RPI_FIRMWARE_VEC_CLK_ID] = {
>   		.export = true,
> @@ -265,7 +285,41 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
>   	return 0;
>   }
>   
> +static int raspberrypi_fw_prepare(struct clk_hw *hw)
> +{
> +	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
> +	struct raspberrypi_clk *rpi = data->rpi;
> +	u32 state = RPI_FIRMWARE_STATE_ENABLE_BIT;
> +	int ret;
> +
> +	ret = raspberrypi_clock_property(rpi->firmware, data,
> +					 RPI_FIRMWARE_SET_CLOCK_STATE, &state);
> +	if (ret)
> +		dev_err_ratelimited(rpi->dev,
> +				    "Failed to set clock %s state to on: %d\n",
> +				    clk_hw_get_name(hw), ret);
> +
> +	return ret;
> +}
> +
> +static void raspberrypi_fw_unprepare(struct clk_hw *hw)
> +{
> +	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
> +	struct raspberrypi_clk *rpi = data->rpi;
> +	u32 state = 0;
> +	int ret;
> +
> +	ret = raspberrypi_clock_property(rpi->firmware, data,
> +					 RPI_FIRMWARE_SET_CLOCK_STATE, &state);
> +	if (ret)
> +		dev_err_ratelimited(rpi->dev,
> +				    "Failed to set clock %s state to off: %d\n",
> +				    clk_hw_get_name(hw), ret);
> +}
> +
>   static const struct clk_ops raspberrypi_firmware_clk_ops = {
> +	.prepare        = raspberrypi_fw_prepare,
> +	.unprepare      = raspberrypi_fw_unprepare,
>   	.is_prepared	= raspberrypi_fw_is_prepared,
>   	.recalc_rate	= raspberrypi_fw_get_rate,
>   	.determine_rate	= raspberrypi_fw_dumb_determine_rate,
> @@ -295,7 +349,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
>   	if (!init.name)
>   		return ERR_PTR(-ENOMEM);
>   	init.ops = &raspberrypi_firmware_clk_ops;
> -	init.flags = CLK_GET_RATE_NOCACHE;
> +	init.flags = variant->flags | CLK_GET_RATE_NOCACHE;
>   
>   	data->hw.init = &init;
>   
>

Re: [PATCH v2 1/5] clk: bcm: rpi: Add missing logs if firmware fails

[Stephen Boyd]

Quoting Maíra Canal (2025-07-31 14:06:17)
> From: Stefan Wahren <wahrenst@gmx.net>
> 
> In contrary to raspberrypi_fw_set_rate(), the ops for is_prepared() and
> recalc_rate() silently ignore firmware errors by just returning 0.
> Since these operations should never fail, add at least error logs
> to inform the user.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---

Applied to clk-next

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Stephen Boyd]

Quoting Maíra Canal (2025-07-31 14:06:18)
> Currently, when we prepare or unprepare RPi's clocks, we don't actually
> enable/disable the firmware clock. This means that
> `clk_disable_unprepare()` doesn't actually change the clock state at
> all, nor does it lowers the clock rate.
> 
> From the Mailbox Property Interface documentation [1], we can see that
> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
> prepare and an unprepare hook for RPi's firmware clock.
> 
> As now the clocks are actually turned off, some of them are now marked
> CLK_IS_CRITICAL, as those are required to be on during the whole system
> operation.
> 
> Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> 
> ---

Applied to clk-next

Re: [PATCH v2 3/5] clk: bcm: rpi: Maximize V3D clock

[Stephen Boyd]

Quoting Maíra Canal (2025-07-31 14:06:19)
> Although minimizing the clock rate is the best for most scenarios, as
> stated in commit 4d85abb0fb8e ("clk: bcm: rpi: Enable minimize for all
> firmware clocks"), when it comes to the GPU, it's ideal to have the
> maximum rate allowed.
> 
> Add an option to maximize a firmware clock's rate when the clock is
> enabled and set this option for V3D.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---

Applied to clk-next

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Marek Szyprowski]

On 31.07.2025 23:06, Maíra Canal wrote:
> Currently, when we prepare or unprepare RPi's clocks, we don't actually
> enable/disable the firmware clock. This means that
> `clk_disable_unprepare()` doesn't actually change the clock state at
> all, nor does it lowers the clock rate.
>
> >From the Mailbox Property Interface documentation [1], we can see that
> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
> prepare and an unprepare hook for RPi's firmware clock.
>
> As now the clocks are actually turned off, some of them are now marked
> CLK_IS_CRITICAL, as those are required to be on during the whole system
> operation.
>
> Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>
> ---
>
> About the pixel clock: currently, if we actually disable the pixel
> clock during a hotplug, the system will crash. This happens in the
> RPi 4.
>
> The crash happens after we disabled the CRTC (thus, the pixel clock),
> but before the end of atomic commit tail. As vc4's pixel valve doesn't
> directly hold a reference to its clock – we use the HDMI encoder to
> manage the pixel clock – I believe we might be disabling the clock
> before we should.
>
> After this investigation, I decided to keep things as they current are:
> the pixel clock is never disabled, as fixing it would go out of
> the scope of this series.
> ---
>   drivers/clk/bcm/clk-raspberrypi.c | 56 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)

This patch landed recently in linux-next as commit 919d6924ae9b ("clk: 
bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). In my 
tests I found that it breaks booting of RaspberryPi3B+ board in ARM 
32bit mode. Surprisingly the same board in ARM 64bit mode correctly 
boots a kernel compiled from the same source. The RPi3B+ board freezes 
after loading the DRM modules (kernel compiled from arm/multi_v7_defconfig):

---->8---

[    7.317423] cfg80211: Loading compiled-in X.509 certificates for 
regulatory database
[    7.379464] Console: switching to colour dummy device 80x30
[    7.407475] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
[    7.434647] input: vc4-hdmi HDMI Jack as 
/devices/platform/soc/3f902000.hdmi/sound/card0/input0
[    7.448937] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
[    7.455677] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
[    7.462371] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4])
[    7.468962] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops 
vc4_crtc_ops [vc4])
[    7.476424] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops 
vc4_crtc_ops [vc4])
[    7.483831] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops 
vc4_crtc_ops [vc4])

(system frozen at this point)


> ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Stefan Wahren]

Hi,

Am 25.09.25 um 09:57 schrieb Marek Szyprowski:
> On 31.07.2025 23:06, Maíra Canal wrote:
>> Currently, when we prepare or unprepare RPi's clocks, we don't actually
>> enable/disable the firmware clock. This means that
>> `clk_disable_unprepare()` doesn't actually change the clock state at
>> all, nor does it lowers the clock rate.
>>
>> >From the Mailbox Property Interface documentation [1], we can see that
>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>> prepare and an unprepare hook for RPi's firmware clock.
>>
>> As now the clocks are actually turned off, some of them are now marked
>> CLK_IS_CRITICAL, as those are required to be on during the whole system
>> operation.
>>
>> Link:https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
>> Signed-off-by: Maíra Canal<mcanal@igalia.com>
>>
>> ---
>>
>> About the pixel clock: currently, if we actually disable the pixel
>> clock during a hotplug, the system will crash. This happens in the
>> RPi 4.
>>
>> The crash happens after we disabled the CRTC (thus, the pixel clock),
>> but before the end of atomic commit tail. As vc4's pixel valve doesn't
>> directly hold a reference to its clock – we use the HDMI encoder to
>> manage the pixel clock – I believe we might be disabling the clock
>> before we should.
>>
>> After this investigation, I decided to keep things as they current are:
>> the pixel clock is never disabled, as fixing it would go out of
>> the scope of this series.
>> ---
>>    drivers/clk/bcm/clk-raspberrypi.c | 56 ++++++++++++++++++++++++++++++++++++++-
>>    1 file changed, 55 insertions(+), 1 deletion(-)
> This patch landed recently in linux-next as commit 919d6924ae9b ("clk:
> bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). In my
> tests I found that it breaks booting of RaspberryPi3B+ board in ARM
> 32bit mode. Surprisingly the same board in ARM 64bit mode correctly
> boots a kernel compiled from the same source. The RPi3B+ board freezes
> after loading the DRM modules (kernel compiled from arm/multi_v7_defconfig):
thanks for spotting and bisecting this. Sorry, I only reviewed the 
changes and didn't had the time to test any affected board.

I was able to reproduce this issue and the following workaround avoid 
the hang in my case:

diff --git a/drivers/clk/bcm/clk-raspberrypi.c 
b/drivers/clk/bcm/clk-raspberrypi.c
index 1a9162f0ae31..94fd4f6e2837 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -137,6 +137,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
         [RPI_FIRMWARE_V3D_CLK_ID] = {
                 .export = true,
                 .maximize = true,
+               .flags = CLK_IS_CRITICAL,
         },
         [RPI_FIRMWARE_PIXEL_CLK_ID] = {
                 .export = true,

The proper fix should be in the clock consumer drivers. I found that 
vc4_v3d doesn't ensure that the clock is enabled before accessing the 
registers. Unfortunately the following change doesn't fix the issue for 
me :-(

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index bb09df5000bd..5e43523732b4 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -441,7 +441,7 @@ static int vc4_v3d_bind(struct device *dev, struct 
device *master, void *data)
         vc4->v3d = v3d;
         v3d->vc4 = vc4;

-       v3d->clk = devm_clk_get_optional(dev, NULL);
+       v3d->clk = devm_clk_get_optional_enabled(dev, NULL);
         if (IS_ERR(v3d->clk))
                 return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to 
get V3D clock\n");

Best regards

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Marek Szyprowski]

On 25.09.2025 18:48, Stefan Wahren wrote:
> Am 25.09.25 um 09:57 schrieb Marek Szyprowski:
>> On 31.07.2025 23:06, Maíra Canal wrote:
>>> Currently, when we prepare or unprepare RPi's clocks, we don't actually
>>> enable/disable the firmware clock. This means that
>>> `clk_disable_unprepare()` doesn't actually change the clock state at
>>> all, nor does it lowers the clock rate.
>>>
>>> >From the Mailbox Property Interface documentation [1], we can see that
>>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>>> prepare and an unprepare hook for RPi's firmware clock.
>>>
>>> As now the clocks are actually turned off, some of them are now marked
>>> CLK_IS_CRITICAL, as those are required to be on during the whole system
>>> operation.
>>>
>>> Link:https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface 
>>> [1]
>>> Signed-off-by: Maíra Canal<mcanal@igalia.com>
>>>
>>> ---
>>>
>>> About the pixel clock: currently, if we actually disable the pixel
>>> clock during a hotplug, the system will crash. This happens in the
>>> RPi 4.
>>>
>>> The crash happens after we disabled the CRTC (thus, the pixel clock),
>>> but before the end of atomic commit tail. As vc4's pixel valve doesn't
>>> directly hold a reference to its clock – we use the HDMI encoder to
>>> manage the pixel clock – I believe we might be disabling the clock
>>> before we should.
>>>
>>> After this investigation, I decided to keep things as they current are:
>>> the pixel clock is never disabled, as fixing it would go out of
>>> the scope of this series.
>>> ---
>>>    drivers/clk/bcm/clk-raspberrypi.c | 56 
>>> ++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 55 insertions(+), 1 deletion(-)
>> This patch landed recently in linux-next as commit 919d6924ae9b ("clk:
>> bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). In my
>> tests I found that it breaks booting of RaspberryPi3B+ board in ARM
>> 32bit mode. Surprisingly the same board in ARM 64bit mode correctly
>> boots a kernel compiled from the same source. The RPi3B+ board freezes
>> after loading the DRM modules (kernel compiled from 
>> arm/multi_v7_defconfig):
> thanks for spotting and bisecting this. Sorry, I only reviewed the 
> changes and didn't had the time to test any affected board.
>
> I was able to reproduce this issue and the following workaround avoid 
> the hang in my case:
>
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c 
> b/drivers/clk/bcm/clk-raspberrypi.c
> index 1a9162f0ae31..94fd4f6e2837 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -137,6 +137,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>         [RPI_FIRMWARE_V3D_CLK_ID] = {
>                 .export = true,
>                 .maximize = true,
> +               .flags = CLK_IS_CRITICAL,
>         },
>         [RPI_FIRMWARE_PIXEL_CLK_ID] = {
>                 .export = true,
>
Right, this fixes (frankly speaking 'hides') the issue. Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> The proper fix should be in the clock consumer drivers. I found that 
> vc4_v3d doesn't ensure that the clock is enabled before accessing the 
> registers. Unfortunately the following change doesn't fix the issue 
> for me :-(
>
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c 
> b/drivers/gpu/drm/vc4/vc4_v3d.c
> index bb09df5000bd..5e43523732b4 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -441,7 +441,7 @@ static int vc4_v3d_bind(struct device *dev, struct 
> device *master, void *data)
>         vc4->v3d = v3d;
>         v3d->vc4 = vc4;
>
> -       v3d->clk = devm_clk_get_optional(dev, NULL);
> +       v3d->clk = devm_clk_get_optional_enabled(dev, NULL);
>         if (IS_ERR(v3d->clk))
>                 return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed 
> to get V3D clock\n");

Well, this can be sorted out in the drivers as a next step.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Stefan Wahren]

Hi Marek,

Am 26.09.25 um 09:27 schrieb Marek Szyprowski:
> On 25.09.2025 18:48, Stefan Wahren wrote:
>> Am 25.09.25 um 09:57 schrieb Marek Szyprowski:
>>> On 31.07.2025 23:06, Maíra Canal wrote:
>>>> Currently, when we prepare or unprepare RPi's clocks, we don't actually
>>>> enable/disable the firmware clock. This means that
>>>> `clk_disable_unprepare()` doesn't actually change the clock state at
>>>> all, nor does it lowers the clock rate.
>>>>
>>>> >From the Mailbox Property Interface documentation [1], we can see that
>>>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>>>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>>>> prepare and an unprepare hook for RPi's firmware clock.
>>>>
>>>> As now the clocks are actually turned off, some of them are now marked
>>>> CLK_IS_CRITICAL, as those are required to be on during the whole system
>>>> operation.
>>>>
>>>> Link:https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface
>>>> [1]
>>>> Signed-off-by: Maíra Canal<mcanal@igalia.com>
>>>>
>>>> ---
>>>>
>>>> About the pixel clock: currently, if we actually disable the pixel
>>>> clock during a hotplug, the system will crash. This happens in the
>>>> RPi 4.
>>>>
>>>> The crash happens after we disabled the CRTC (thus, the pixel clock),
>>>> but before the end of atomic commit tail. As vc4's pixel valve doesn't
>>>> directly hold a reference to its clock – we use the HDMI encoder to
>>>> manage the pixel clock – I believe we might be disabling the clock
>>>> before we should.
>>>>
>>>> After this investigation, I decided to keep things as they current are:
>>>> the pixel clock is never disabled, as fixing it would go out of
>>>> the scope of this series.
>>>> ---
>>>>     drivers/clk/bcm/clk-raspberrypi.c | 56
>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>     1 file changed, 55 insertions(+), 1 deletion(-)
>>> This patch landed recently in linux-next as commit 919d6924ae9b ("clk:
>>> bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). In my
>>> tests I found that it breaks booting of RaspberryPi3B+ board in ARM
>>> 32bit mode. Surprisingly the same board in ARM 64bit mode correctly
>>> boots a kernel compiled from the same source. The RPi3B+ board freezes
>>> after loading the DRM modules (kernel compiled from
>>> arm/multi_v7_defconfig):
>> thanks for spotting and bisecting this. Sorry, I only reviewed the
>> changes and didn't had the time to test any affected board.
>>
>> I was able to reproduce this issue and the following workaround avoid
>> the hang in my case:
>>
>> diff --git a/drivers/clk/bcm/clk-raspberrypi.c
>> b/drivers/clk/bcm/clk-raspberrypi.c
>> index 1a9162f0ae31..94fd4f6e2837 100644
>> --- a/drivers/clk/bcm/clk-raspberrypi.c
>> +++ b/drivers/clk/bcm/clk-raspberrypi.c
>> @@ -137,6 +137,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>          [RPI_FIRMWARE_V3D_CLK_ID] = {
>>                  .export = true,
>>                  .maximize = true,
>> +               .flags = CLK_IS_CRITICAL,
>>          },
>>          [RPI_FIRMWARE_PIXEL_CLK_ID] = {
>>                  .export = true,
>>
> Right, this fixes (frankly speaking 'hides') the issue. Feel free to add:
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
AFAIK the offending clock change isn't in the downstream kernel, so I 
like to see the opinion of María and the Raspberry Pi people first.

Currently I know that in the error case the following clocks are 
disabled during boot of Raspberry Pi 3B+:
fw-clk-vec
fw-clk-isp
fw-clk-v3d

So it's very likely that the vc4 drivers tries to access the register 
after the these clocks has been disabled and then the system freeze. The 
workaround above was just a wild guess, so currently I don't know why 
this change avoid the freeze.

Best regards

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Melissa Wen]

On 26/09/2025 07:36, Stefan Wahren wrote:
> Hi Marek,
>
> Am 26.09.25 um 09:27 schrieb Marek Szyprowski:
>> On 25.09.2025 18:48, Stefan Wahren wrote:
>>> Am 25.09.25 um 09:57 schrieb Marek Szyprowski:
>>>> On 31.07.2025 23:06, Maíra Canal wrote:
>>>>> Currently, when we prepare or unprepare RPi's clocks, we don't 
>>>>> actually
>>>>> enable/disable the firmware clock. This means that
>>>>> `clk_disable_unprepare()` doesn't actually change the clock state at
>>>>> all, nor does it lowers the clock rate.
>>>>>
>>>>> >From the Mailbox Property Interface documentation [1], we can see 
>>>>> that
>>>>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>>>>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>>>>> prepare and an unprepare hook for RPi's firmware clock.
>>>>>
>>>>> As now the clocks are actually turned off, some of them are now 
>>>>> marked
>>>>> CLK_IS_CRITICAL, as those are required to be on during the whole 
>>>>> system
>>>>> operation.
>>>>>
>>>>> Link:https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface 
>>>>>
>>>>> [1]
>>>>> Signed-off-by: Maíra Canal<mcanal@igalia.com>
>>>>>
>>>>> ---
>>>>>
>>>>> About the pixel clock: currently, if we actually disable the pixel
>>>>> clock during a hotplug, the system will crash. This happens in the
>>>>> RPi 4.
>>>>>
>>>>> The crash happens after we disabled the CRTC (thus, the pixel clock),
>>>>> but before the end of atomic commit tail. As vc4's pixel valve 
>>>>> doesn't
>>>>> directly hold a reference to its clock – we use the HDMI encoder to
>>>>> manage the pixel clock – I believe we might be disabling the clock
>>>>> before we should.
>>>>>
>>>>> After this investigation, I decided to keep things as they current 
>>>>> are:
>>>>> the pixel clock is never disabled, as fixing it would go out of
>>>>> the scope of this series.
>>>>> ---
>>>>>     drivers/clk/bcm/clk-raspberrypi.c | 56
>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>     1 file changed, 55 insertions(+), 1 deletion(-)
>>>> This patch landed recently in linux-next as commit 919d6924ae9b ("clk:
>>>> bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). 
>>>> In my
>>>> tests I found that it breaks booting of RaspberryPi3B+ board in ARM
>>>> 32bit mode. Surprisingly the same board in ARM 64bit mode correctly
>>>> boots a kernel compiled from the same source. The RPi3B+ board freezes
>>>> after loading the DRM modules (kernel compiled from
>>>> arm/multi_v7_defconfig):
>>> thanks for spotting and bisecting this. Sorry, I only reviewed the
>>> changes and didn't had the time to test any affected board.
>>>
>>> I was able to reproduce this issue and the following workaround avoid
>>> the hang in my case:
>>>
>>> diff --git a/drivers/clk/bcm/clk-raspberrypi.c
>>> b/drivers/clk/bcm/clk-raspberrypi.c
>>> index 1a9162f0ae31..94fd4f6e2837 100644
>>> --- a/drivers/clk/bcm/clk-raspberrypi.c
>>> +++ b/drivers/clk/bcm/clk-raspberrypi.c
>>> @@ -137,6 +137,7 @@ 
>>> raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>>          [RPI_FIRMWARE_V3D_CLK_ID] = {
>>>                  .export = true,
>>>                  .maximize = true,
>>> +               .flags = CLK_IS_CRITICAL,
>>>          },
>>>          [RPI_FIRMWARE_PIXEL_CLK_ID] = {
>>>                  .export = true,
>>>
>> Right, this fixes (frankly speaking 'hides') the issue. Feel free to 
>> add:
>>
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
> AFAIK the offending clock change isn't in the downstream kernel, so I 
> like to see the opinion of María and the Raspberry Pi people first.

Hi,

I see in the downstream kernel the CLOCK_V3D was removed in favor of 
firmware clock:
https://github.com/raspberrypi/linux/blob/rpi-6.12.y/drivers/clk/bcm/clk-bcm2835.c#L2076

Also, v3d in RPi4 is set to use the firmware clock:
https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi#L97

I think v3d clock is missed on boot, but I also think the issue should 
be solved by setting the v3d firmware clock for Pi3.
WDYT? Can you check it on your side? Something like:

diff --git a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi 
b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
index 8b3c21d9f333..3289cb5dfa8e 100644
--- a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
@@ -14,6 +14,7 @@ &hdmi {
  };

  &v3d {
+       clocks = <&firmware_clocks 5>;
         power-domains = <&power RPI_POWER_DOMAIN_V3D>;
  };

Best regards,

Melissa

>
> Currently I know that in the error case the following clocks are 
> disabled during boot of Raspberry Pi 3B+:
> fw-clk-vec
> fw-clk-isp
> fw-clk-v3d
>
> So it's very likely that the vc4 drivers tries to access the register 
> after the these clocks has been disabled and then the system freeze. 
> The workaround above was just a wild guess, so currently I don't know 
> why this change avoid the freeze.
>
> Best regards

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Marek Szyprowski]

On 01.10.2025 22:50, Melissa Wen wrote:
> On 26/09/2025 07:36, Stefan Wahren wrote:
>> Am 26.09.25 um 09:27 schrieb Marek Szyprowski:
>>> On 25.09.2025 18:48, Stefan Wahren wrote:
>>>> Am 25.09.25 um 09:57 schrieb Marek Szyprowski:
>>>>> On 31.07.2025 23:06, Maíra Canal wrote:
>>>>>> Currently, when we prepare or unprepare RPi's clocks, we don't 
>>>>>> actually
>>>>>> enable/disable the firmware clock. This means that
>>>>>> `clk_disable_unprepare()` doesn't actually change the clock state at
>>>>>> all, nor does it lowers the clock rate.
>>>>>>
>>>>>> >From the Mailbox Property Interface documentation [1], we can 
>>>>>> see that
>>>>>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>>>>>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>>>>>> prepare and an unprepare hook for RPi's firmware clock.
>>>>>>
>>>>>> As now the clocks are actually turned off, some of them are now 
>>>>>> marked
>>>>>> CLK_IS_CRITICAL, as those are required to be on during the whole 
>>>>>> system
>>>>>> operation.
>>>>>>
>>>>>> Link:https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface 
>>>>>>
>>>>>> [1]
>>>>>> Signed-off-by: Maíra Canal<mcanal@igalia.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> About the pixel clock: currently, if we actually disable the pixel
>>>>>> clock during a hotplug, the system will crash. This happens in the
>>>>>> RPi 4.
>>>>>>
>>>>>> The crash happens after we disabled the CRTC (thus, the pixel 
>>>>>> clock),
>>>>>> but before the end of atomic commit tail. As vc4's pixel valve 
>>>>>> doesn't
>>>>>> directly hold a reference to its clock – we use the HDMI encoder to
>>>>>> manage the pixel clock – I believe we might be disabling the clock
>>>>>> before we should.
>>>>>>
>>>>>> After this investigation, I decided to keep things as they 
>>>>>> current are:
>>>>>> the pixel clock is never disabled, as fixing it would go out of
>>>>>> the scope of this series.
>>>>>> ---
>>>>>>     drivers/clk/bcm/clk-raspberrypi.c | 56
>>>>>> ++++++++++++++++++++++++++++++++++++++-
>>>>>>     1 file changed, 55 insertions(+), 1 deletion(-)
>>>>> This patch landed recently in linux-next as commit 919d6924ae9b 
>>>>> ("clk:
>>>>> bcm: rpi: Turn firmware clock on/off when preparing/unpreparing"). 
>>>>> In my
>>>>> tests I found that it breaks booting of RaspberryPi3B+ board in ARM
>>>>> 32bit mode. Surprisingly the same board in ARM 64bit mode correctly
>>>>> boots a kernel compiled from the same source. The RPi3B+ board 
>>>>> freezes
>>>>> after loading the DRM modules (kernel compiled from
>>>>> arm/multi_v7_defconfig):
>>>> thanks for spotting and bisecting this. Sorry, I only reviewed the
>>>> changes and didn't had the time to test any affected board.
>>>>
>>>> I was able to reproduce this issue and the following workaround avoid
>>>> the hang in my case:
>>>>
>>>> diff --git a/drivers/clk/bcm/clk-raspberrypi.c
>>>> b/drivers/clk/bcm/clk-raspberrypi.c
>>>> index 1a9162f0ae31..94fd4f6e2837 100644
>>>> --- a/drivers/clk/bcm/clk-raspberrypi.c
>>>> +++ b/drivers/clk/bcm/clk-raspberrypi.c
>>>> @@ -137,6 +137,7 @@ 
>>>> raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>>>          [RPI_FIRMWARE_V3D_CLK_ID] = {
>>>>                  .export = true,
>>>>                  .maximize = true,
>>>> +               .flags = CLK_IS_CRITICAL,
>>>>          },
>>>>          [RPI_FIRMWARE_PIXEL_CLK_ID] = {
>>>>                  .export = true,
>>>>
>>> Right, this fixes (frankly speaking 'hides') the issue. Feel free to 
>>> add:
>>>
>>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>> AFAIK the offending clock change isn't in the downstream kernel, so I 
>> like to see the opinion of María and the Raspberry Pi people first.
>
> Hi,
>
> I see in the downstream kernel the CLOCK_V3D was removed in favor of 
> firmware clock:
> https://github.com/raspberrypi/linux/blob/rpi-6.12.y/drivers/clk/bcm/clk-bcm2835.c#L2076 
>
>
> Also, v3d in RPi4 is set to use the firmware clock:
> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi#L97 
>
>
> I think v3d clock is missed on boot, but I also think the issue should 
> be solved by setting the v3d firmware clock for Pi3.
> WDYT? Can you check it on your side? Something like:
>
> diff --git a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi 
> b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
> index 8b3c21d9f333..3289cb5dfa8e 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
> @@ -14,6 +14,7 @@ &hdmi {
>  };
>
>  &v3d {
> +       clocks = <&firmware_clocks 5>;
>         power-domains = <&power RPI_POWER_DOMAIN_V3D>;
>  };
>
This works for me and fixes the mentioned issue. Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/5] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Stefan Wahren]

Hi,

Am 01.10.25 um 22:50 schrieb Melissa Wen:
>
>
> On 26/09/2025 07:36, Stefan Wahren wrote:
>> Hi Marek,
>>
>> ....
>> AFAIK the offending clock change isn't in the downstream kernel, so I 
>> like to see the opinion of María and the Raspberry Pi people first.
>
> Hi,
>
> I see in the downstream kernel the CLOCK_V3D was removed in favor of 
> firmware clock:
> https://github.com/raspberrypi/linux/blob/rpi-6.12.y/drivers/clk/bcm/clk-bcm2835.c#L2076 
>
>
> Also, v3d in RPi4 is set to use the firmware clock:
> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi#L97 
>
>
> I think v3d clock is missed on boot, but I also think the issue should 
> be solved by setting the v3d firmware clock for Pi3.
> WDYT? Can you check it on your side? Something like:
>
> diff --git a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi 
> b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
> index 8b3c21d9f333..3289cb5dfa8e 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2835-rpi-common.dtsi
> @@ -14,6 +14,7 @@ &hdmi {
>  };
>
>  &v3d {
> +       clocks = <&firmware_clocks 5>;
>         power-domains = <&power RPI_POWER_DOMAIN_V3D>;
>  };
thanks for pointing out. Handling the same clock by clk-raspberrypi and 
clk-bcm2835 is a very bad idea. But it looks like that's not the only 
affected clock. I see at least ISP and VEC, which might be affected.

Does anyone know, if the clk-raspberrypi always use a matching clock 
name in comparison to the clk-bcm2835?
clk-bcm2835 - clk-raspberrypi
BCM2835_CLOCK_V3D equals to RPI_FIRMWARE_V3D_CLK_ID
BCM2835_CLOCK_ISP equals to RPI_FIRMWARE_ISP_CLK_ID
...
Or are there any clocks, which have different names in both drivers but 
describe the same hardware clock?

Best regards
>
> Best regards,
>
> Melissa
>
>>
>> Currently I know that in the error case the following clocks are 
>> disabled during boot of Raspberry Pi 3B+:
>> fw-clk-vec
>> fw-clk-isp
>> fw-clk-v3d
>>
>> So it's very likely that the vc4 drivers tries to access the register 
>> after the these clocks has been disabled and then the system freeze. 
>> The workaround above was just a wild guess, so currently I don't know 
>> why this change avoid the freeze.
>>
>> Best regards
>