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

[PATCH 0/3] 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 1/3 addresses this issue in
clk/bcm/clk-raspberrypi.c.

The other two 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

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

 drivers/clk/bcm/clk-raspberrypi.c |  41 ++++++++-
 drivers/gpu/drm/v3d/Makefile      |   3 +-
 drivers/gpu/drm/v3d/v3d_debugfs.c |  23 ++++-
 drivers/gpu/drm/v3d/v3d_drv.c     | 171 +++++++++++++++++++-------------------
 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, 291 insertions(+), 111 deletions(-)
---
base-commit: a7352c849492a30b5d8491fcb9314ab376a3942f
change-id: 20250728-v3d-power-management-eebb2024dc96

[PATCH 1/3] 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
with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
early boot or are required during reboot.

Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
Fixes: 93d2725affd6 ("clk: bcm: rpi: Discover the firmware clocks")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/clk/bcm/clk-raspberrypi.c | 41 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 8e4fde03ed232b464165f524d27744b4ced93a60..a2bd5040283a2f456760bd685e696b423985cac0 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_IGNORE_UNUSED,
 	},
 	[RPI_FIRMWARE_CORE_CLK_ID] = {
 		.export = true,
@@ -90,6 +92,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 		 * always use the minimum the drivers will let us.
 		 */
 		.minimize = true,
+		.flags = CLK_IGNORE_UNUSED,
 	},
 	[RPI_FIRMWARE_M2MC_CLK_ID] = {
 		.export = true,
@@ -115,6 +118,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 		 * drivers will let us.
 		 */
 		.minimize = true,
+		.flags = CLK_IGNORE_UNUSED,
 	},
 	[RPI_FIRMWARE_V3D_CLK_ID] = {
 		.export = true,
@@ -127,6 +131,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
 	[RPI_FIRMWARE_HEVC_CLK_ID] = {
 		.export = true,
 		.minimize = true,
+		.flags = CLK_IGNORE_UNUSED,
 	},
 	[RPI_FIRMWARE_ISP_CLK_ID] = {
 		.export = true,
@@ -135,6 +140,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,
@@ -259,7 +265,40 @@ 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(rpi->dev, "Failed to set clock %d state to on: %d",
+			data->id, 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(rpi->dev, "Failed to set clock %d state to off: %d",
+			data->id, 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,
@@ -289,7 +328,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 2/3] 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.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.c | 92 ++++++++++++++++++++++---------------------
 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, 66 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 2def155ce496ec5f159f6bda9929aeaae141d1a6..6e6b830bee6587e4170fd64d354916766e59d2e5 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -347,14 +347,55 @@ 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_exclusive(dev, NULL);
+	if (IS_ERR(v3d->reset)) {
+		ret = PTR_ERR(v3d->reset);
+
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		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");
+			return ret;
+		}
+	}
+
+	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 = v3d_irq_init(v3d);
+	if (ret)
+		goto gem_destroy;
+
+	v3d_perfmon_init(v3d);
+
 	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");
+	if (IS_ERR(v3d->clk)) {
+		ret = dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
+		goto gem_destroy;
+	}
 
 	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 +422,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 +439,12 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	drm_dev_unregister(drm);
 irq_disable:
 	v3d_irq_disable(v3d);
+clk_disable:
+	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;
 }
 
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 3/3] 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     | 85 ++++++++++++++++++---------------------
 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, 189 insertions(+), 56 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 6e6b830bee6587e4170fd64d354916766e59d2e5..c937d4d738313790da30294f9e094205ff0d4fc0 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)
 {
@@ -392,19 +370,26 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 		goto gem_destroy;
 	}
 
-	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)
+			return ret;
+	}
 
 	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);
 
@@ -423,24 +408,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:
-	clk_disable_unprepare(v3d->clk);
+runtime_pm_put:
+	pm_runtime_put_sync_suspend(dev);
 gem_destroy:
 	v3d_gem_destroy(drm);
 dma_free:
@@ -460,20 +448,25 @@ static void v3d_platform_drm_remove(struct platform_device *pdev)
 
 	v3d_gem_destroy(drm);
 
-	dma_free_wc(v3d->drm.dev, 4096, v3d->mmu_scratch,
-		    v3d->mmu_scratch_paddr);
+	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
 
-	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);
 }
 
+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 2/3] drm/v3d: Allocate all resources before enabling the clock

[Philipp Zabel]

Hi Maíra,

On Mo, 2025-07-28 at 09:35 -0300, Maíra Canal wrote:
> Move all resource allocation operations before actually enabling the
> clock,

This patch moves code even before requesting the clock.
But I don't think this is necessary, see below.

> as those operation don't require the GPU to be powered on.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/v3d/v3d_drv.c | 92 ++++++++++++++++++++++---------------------
>  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, 66 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index 2def155ce496ec5f159f6bda9929aeaae141d1a6..6e6b830bee6587e4170fd64d354916766e59d2e5 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -347,14 +347,55 @@ 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_exclusive(dev, NULL);
> +	if (IS_ERR(v3d->reset)) {
> +		ret = PTR_ERR(v3d->reset);
> +
> +		if (ret == -EPROBE_DEFER)
> +			return ret;
> +
> +		v3d->reset = NULL;

Drive-by comment, not an issue with this (code-moving) patch: It looks
like this open-codes devm_reset_control_get_optional_exclusive().

> +		ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to get reset control or bridge regs\n");
> +			return ret;
> +		}
> +	}
> +
> +	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 = v3d_irq_init(v3d);
> +	if (ret)
> +		goto gem_destroy;

These functions needing manual cleanup are called before another devm_
function below. With this, resources are not freed in inverse order of
allocation. I don't see whether mixing devm and non-devm initialization
is an actual problem in this case, but it would look cleaner if the
devm_clk_get_optional() below was just moved back up before
dma_alloc_wc().

If there are also devm_ functions called from inside the v3d_
functions, it might be better to move all cleanup into devm actions.

> +	v3d_perfmon_init(v3d);
> +
>  	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");
> +	if (IS_ERR(v3d->clk)) {
> +		ret = dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
> +		goto gem_destroy;
> +	}
>  
>  	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 +422,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 +439,12 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
>  	drm_dev_unregister(drm);
>  irq_disable:
>  	v3d_irq_disable(v3d);
> +clk_disable:
> +	clk_disable_unprepare(v3d->clk);

clk_disable_unprepare() is moved up in the error path, but not in
v3d_platform_drm_remove(). Should this be kept consistent?

>  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;
>  }

regards
Philipp

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Stefan Wahren]

Hi Maíra,

thanks for working on this.

Am 28.07.25 um 14:35 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
> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
> early boot or are required during reboot.
>
> Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface [1]
> Fixes: 93d2725affd6 ("clk: bcm: rpi: Discover the firmware clocks")
could you please explain from user perspective, which issue is fixed by 
this patch?

Why does this needs to be backported?
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   drivers/clk/bcm/clk-raspberrypi.c | 41 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index 8e4fde03ed232b464165f524d27744b4ced93a60..a2bd5040283a2f456760bd685e696b423985cac0 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_IGNORE_UNUSED,
>   	},
>   	[RPI_FIRMWARE_CORE_CLK_ID] = {
>   		.export = true,
> @@ -90,6 +92,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   		 * always use the minimum the drivers will let us.
>   		 */
>   		.minimize = true,
> +		.flags = CLK_IGNORE_UNUSED,
>   	},
>   	[RPI_FIRMWARE_M2MC_CLK_ID] = {
>   		.export = true,
> @@ -115,6 +118,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   		 * drivers will let us.
>   		 */
>   		.minimize = true,
> +		.flags = CLK_IGNORE_UNUSED,
>   	},
>   	[RPI_FIRMWARE_V3D_CLK_ID] = {
>   		.export = true,
> @@ -127,6 +131,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>   	[RPI_FIRMWARE_HEVC_CLK_ID] = {
>   		.export = true,
>   		.minimize = true,
> +		.flags = CLK_IGNORE_UNUSED,
>   	},
>   	[RPI_FIRMWARE_ISP_CLK_ID] = {
>   		.export = true,
> @@ -135,6 +140,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,
> @@ -259,7 +265,40 @@ 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(rpi->dev, "Failed to set clock %d state to on: %d",
> +			data->id, ret);
I suggest to use dev_err_ratelimited for prepare/unprepare, otherwise 
this could spam the kernel log.

Furthermore i wouldn't recommend to log some magic clock id. How about 
using clk_hw_get_name(hw) instead?

Don't we need a newline character at the end?

> +
> +	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(rpi->dev, "Failed to set clock %d state to off: %d",
> +			data->id, ret);
see above

Best regards
> +}
> +
> +
>   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,
> @@ -289,7 +328,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 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maíra Canal]

Hi Stefan,

On 28/07/25 13:33, Stefan Wahren wrote:
> Hi Maíra,
> 
> thanks for working on this.
> 
> Am 28.07.25 um 14:35 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
>> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
>> early boot or are required during reboot.
>>
>> Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property- 
>> interface [1]
>> Fixes: 93d2725affd6 ("clk: bcm: rpi: Discover the firmware clocks")
> could you please explain from user perspective, which issue is fixed by 
> this patch?

I was about to talk about the power savings benefits for the user.
However, as I type, I notice that such a thing doesn't justify a
"Fixes:" tag. I'll drop it.

Thanks for your review, I'll address all the comments.

Best Regards,
- Maíra

> 
> Why does this needs to be backported?
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/clk/bcm/clk-raspberrypi.c | 41 +++++++++++++++++++++++++++++ 
>> +++++++++-
>>   1 file changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk- 
>> raspberrypi.c
>> index 
>> 8e4fde03ed232b464165f524d27744b4ced93a60..a2bd5040283a2f456760bd685e696b423985cac0 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_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_CORE_CLK_ID] = {
>>           .export = true,
>> @@ -90,6 +92,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>            * always use the minimum the drivers will let us.
>>            */
>>           .minimize = true,
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_M2MC_CLK_ID] = {
>>           .export = true,
>> @@ -115,6 +118,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>            * drivers will let us.
>>            */
>>           .minimize = true,
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_V3D_CLK_ID] = {
>>           .export = true,
>> @@ -127,6 +131,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>       [RPI_FIRMWARE_HEVC_CLK_ID] = {
>>           .export = true,
>>           .minimize = true,
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_ISP_CLK_ID] = {
>>           .export = true,
>> @@ -135,6 +140,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,
>> @@ -259,7 +265,40 @@ 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(rpi->dev, "Failed to set clock %d state to on: %d",
>> +            data->id, ret);
> I suggest to use dev_err_ratelimited for prepare/unprepare, otherwise 
> this could spam the kernel log.
> 
> Furthermore i wouldn't recommend to log some magic clock id. How about 
> using clk_hw_get_name(hw) instead?
> 
> Don't we need a newline character at the end?
> 
>> +
>> +    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(rpi->dev, "Failed to set clock %d state to off: %d",
>> +            data->id, ret);
> see above
> 
> Best regards
>> +}
>> +
>> +
>>   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,
>> @@ -289,7 +328,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 0/3] Power Management for Raspberry Pi V3D GPU

[Stefan Wahren]

Hi,

Am 28.07.25 um 14:35 schrieb 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 1/3 addresses this issue in
> clk/bcm/clk-raspberrypi.c.
>
> The other two patches are related to PM enablement in the V3D driver.
Maybe you want to cherry-pick this older patch and integrate it into 
your series? [2]

[2] - 
https://github.com/lategoodbye/linux-dev/commit/2ee5e1205922b06100206e760ed8aefe0b6d322f
>
> 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
>
> ---
> Maíra Canal (3):
>        clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing
>        drm/v3d: Allocate all resources before enabling the clock
>        drm/v3d: Introduce Runtime Power Management
>
>   drivers/clk/bcm/clk-raspberrypi.c |  41 ++++++++-
>   drivers/gpu/drm/v3d/Makefile      |   3 +-
>   drivers/gpu/drm/v3d/v3d_debugfs.c |  23 ++++-
>   drivers/gpu/drm/v3d/v3d_drv.c     | 171 +++++++++++++++++++-------------------
>   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, 291 insertions(+), 111 deletions(-)
> ---
> base-commit: a7352c849492a30b5d8491fcb9314ab376a3942f
> change-id: 20250728-v3d-power-management-eebb2024dc96
>

Re: [PATCH 0/3] Power Management for Raspberry Pi V3D GPU

[Maíra Canal]

Hi Stefan,

On 28/07/25 18:00, Stefan Wahren wrote:
> Hi,
> 
> Am 28.07.25 um 14:35 schrieb 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 1/3 addresses this issue in
>> clk/bcm/clk-raspberrypi.c.
>>
>> The other two patches are related to PM enablement in the V3D driver.
> Maybe you want to cherry-pick this older patch and integrate it into 
> your series? [2]

Yeah, sure! I'll add your patch to v2.

Best Regards,
- Maíra

> 
> [2] - https://github.com/lategoodbye/linux-dev/ 
> commit/2ee5e1205922b06100206e760ed8aefe0b6d322f
>>
>> 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
>>
>> ---
>> Maíra Canal (3):
>>        clk: bcm: rpi: Turn firmware clock on/off when preparing/ 
>> unpreparing
>>        drm/v3d: Allocate all resources before enabling the clock
>>        drm/v3d: Introduce Runtime Power Management
>>
>>   drivers/clk/bcm/clk-raspberrypi.c |  41 ++++++++-
>>   drivers/gpu/drm/v3d/Makefile      |   3 +-
>>   drivers/gpu/drm/v3d/v3d_debugfs.c |  23 ++++-
>>   drivers/gpu/drm/v3d/v3d_drv.c     | 171 ++++++++++++++++++ 
>> +-------------------
>>   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, 291 insertions(+), 111 deletions(-)
>> ---
>> base-commit: a7352c849492a30b5d8491fcb9314ab376a3942f
>> change-id: 20250728-v3d-power-management-eebb2024dc96
>>
> 

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

Hi Maíra,

On Mon, Jul 28, 2025 at 09:35:38AM -0300, 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
> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
> early boot or are required during reboot.

What difference is there between the CLK_IGNORE_UNUSED and
CLK_IS_CRITICAL clocks?

I'm asking, because CLK_IGNORE_UNUSED is mostly useless, and
CLK_IS_CRITICAL is probably what you're looking for for all of them.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maíra Canal]

Hi Maxime,

On 29/07/25 04:27, Maxime Ripard wrote:
> Hi Maíra,
> 
> On Mon, Jul 28, 2025 at 09:35:38AM -0300, 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
>> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
>> early boot or are required during reboot.
> 
> What difference is there between the CLK_IGNORE_UNUSED and
> CLK_IS_CRITICAL clocks?

 From my understanding, CLK_IGNORE_UNUSED will prevent the clock to be
gated during boot (on "clk: Disabling unused clocks"), but after it, the
clock can be gated.

With CLK_IS_CRITICAL, the clock will never be disabled.

For example, RPI_FIRMWARE_M2MC_CLK_ID is used by vc4. It needs to be
enabled at boot (I tested; if not enabled, it won't boot). However,
after vc4 is probed, we would like vc4 to have control of it and be able
to unprepare it in `vc4_hdmi_runtime_suspend()`. If I set it as
CLK_IS_CRITICAL, vc4 won't be able to unprepare it.

I only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
RPi won't reboot.

Best Regards,
- Maíra

> 
> I'm asking, because CLK_IGNORE_UNUSED is mostly useless, and
> CLK_IS_CRITICAL is probably what you're looking for for all of them.
> 
> Maxime

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

On Tue, Jul 29, 2025 at 08:53:51AM -0300, Maíra Canal wrote:
> Hi Maxime,
> 
> On 29/07/25 04:27, Maxime Ripard wrote:
> > Hi Maíra,
> > 
> > On Mon, Jul 28, 2025 at 09:35:38AM -0300, 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
> > > with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
> > > early boot or are required during reboot.
> > 
> > What difference is there between the CLK_IGNORE_UNUSED and
> > CLK_IS_CRITICAL clocks?
> 
> From my understanding, CLK_IGNORE_UNUSED will prevent the clock to be
> gated during boot (on "clk: Disabling unused clocks"), but after it, the
> clock can be gated.
> 
> With CLK_IS_CRITICAL, the clock will never be disabled.

Yeah, that's correct.

> For example, RPI_FIRMWARE_M2MC_CLK_ID is used by vc4. It needs to be
> enabled at boot (I tested; if not enabled, it won't boot). However,
> after vc4 is probed, we would like vc4 to have control of it and be able
> to unprepare it in `vc4_hdmi_runtime_suspend()`. If I set it as
> CLK_IS_CRITICAL, vc4 won't be able to unprepare it.

If the clock can be disabled by Linux, but it breaks some drivers if
it's not enabled during their probe, something is fishy somewhere, and
it's likely it would be just as broken if you compiled the driver as a
module.

Even then, some of the other clocks should probably never be disabled,
like the CPU clock.

> I only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
> RPi won't reboot.

Why?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maíra Canal]

Hi Maxime,

On 29/07/25 09:14, Maxime Ripard wrote:
> On Tue, Jul 29, 2025 at 08:53:51AM -0300, Maíra Canal wrote:
>> Hi Maxime,
>>
>> On 29/07/25 04:27, Maxime Ripard wrote:
>>> Hi Maíra,
>>>
>>> On Mon, Jul 28, 2025 at 09:35:38AM -0300, 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
>>>> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
>>>> early boot or are required during reboot.
>>>
>>> What difference is there between the CLK_IGNORE_UNUSED and
>>> CLK_IS_CRITICAL clocks?
>>
>>  From my understanding, CLK_IGNORE_UNUSED will prevent the clock to be
>> gated during boot (on "clk: Disabling unused clocks"), but after it, the
>> clock can be gated.
>>
>> With CLK_IS_CRITICAL, the clock will never be disabled.
> 
> Yeah, that's correct.
> 
>> For example, RPI_FIRMWARE_M2MC_CLK_ID is used by vc4. It needs to be
>> enabled at boot (I tested; if not enabled, it won't boot). However,
>> after vc4 is probed, we would like vc4 to have control of it and be able
>> to unprepare it in `vc4_hdmi_runtime_suspend()`. If I set it as
>> CLK_IS_CRITICAL, vc4 won't be able to unprepare it.
> 
> If the clock can be disabled by Linux, but it breaks some drivers if
> it's not enabled during their probe, something is fishy somewhere, and
> it's likely it would be just as broken if you compiled the driver as a
> module.
> 
> Even then, some of the other clocks should probably never be disabled,
> like the CPU clock.

I'll mark RPI_FIRMWARE_ARM_CLK_ID and RPI_FIRMWARE_CORE_CLK_ID as
critical. Are there any other clocks you think should never be disabled?

> 
>> I only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
>> RPi won't reboot.
> 
> Why?

I'll have to dig a bit into vc4 HDMI code and to investigate the reason
(and maybe fix the issue there).

Best Regards,
- Maíra

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maíra Canal]

On 29/07/25 13:19, Maíra Canal wrote:
> Hi Maxime,
> 
> On 29/07/25 09:14, Maxime Ripard wrote:
>> On Tue, Jul 29, 2025 at 08:53:51AM -0300, Maíra Canal wrote:
>>> Hi Maxime,
>>>
>>> On 29/07/25 04:27, Maxime Ripard wrote:
>>>> Hi Maíra,
>>>>
>>>> On Mon, Jul 28, 2025 at 09:35:38AM -0300, 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
>>>>> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
>>>>> early boot or are required during reboot.
>>>>
>>>> What difference is there between the CLK_IGNORE_UNUSED and
>>>> CLK_IS_CRITICAL clocks?
>>>
>>>  From my understanding, CLK_IGNORE_UNUSED will prevent the clock to be
>>> gated during boot (on "clk: Disabling unused clocks"), but after it, the
>>> clock can be gated.
>>>
>>> With CLK_IS_CRITICAL, the clock will never be disabled.
>>
>> Yeah, that's correct.
>>
>>> For example, RPI_FIRMWARE_M2MC_CLK_ID is used by vc4. It needs to be
>>> enabled at boot (I tested; if not enabled, it won't boot). However,
>>> after vc4 is probed, we would like vc4 to have control of it and be able
>>> to unprepare it in `vc4_hdmi_runtime_suspend()`. If I set it as
>>> CLK_IS_CRITICAL, vc4 won't be able to unprepare it.
>>
>> If the clock can be disabled by Linux, but it breaks some drivers if
>> it's not enabled during their probe, something is fishy somewhere, and
>> it's likely it would be just as broken if you compiled the driver as a
>> module.
>>
>> Even then, some of the other clocks should probably never be disabled,
>> like the CPU clock.
> 
> I'll mark RPI_FIRMWARE_ARM_CLK_ID and RPI_FIRMWARE_CORE_CLK_ID as
> critical. Are there any other clocks you think should never be disabled?
> 
>>
>>> I only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
>>> RPi won't reboot.
>>
>> Why?
> 
> I'll have to dig a bit into vc4 HDMI code and to investigate the reason
> (and maybe fix the issue there).

After some investigation, I believe that those display-related should be
set to CLK_IGNORE_UNUSED. It's not that it breaks some drivers if not
enabled, but it breaks hardware functionality and the device won't boot.
See, for example, clk-bcm2835 in which all PLL and PLL dividers clocks
are marked with CLK_IGNORE_UNUSED and some with CLK_IS_CRITICAL.

Maybe Dave has some input about the topic?

So far, I'm planing to keep CLK_IGNORE_UNUSED to the display-related
clocks and remove CLK_IS_CRITICAL from RPI_FIRMWARE_PIXEL_BVB_CLK_ID. If
you have any objections about it, let me know.

Best Regards,
- Maíra

> 
> Best Regards,
> - Maíra
> 

Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4200 bytes --]

On Tue, Jul 29, 2025 at 09:33:50PM -0300, Maíra Canal wrote:
> On 29/07/25 13:19, Maíra Canal wrote:
> > Hi Maxime,
> > 
> > On 29/07/25 09:14, Maxime Ripard wrote:
> > > On Tue, Jul 29, 2025 at 08:53:51AM -0300, Maíra Canal wrote:
> > > > Hi Maxime,
> > > > 
> > > > On 29/07/25 04:27, Maxime Ripard wrote:
> > > > > Hi Maíra,
> > > > > 
> > > > > On Mon, Jul 28, 2025 at 09:35:38AM -0300, 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
> > > > > > with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
> > > > > > early boot or are required during reboot.
> > > > > 
> > > > > What difference is there between the CLK_IGNORE_UNUSED and
> > > > > CLK_IS_CRITICAL clocks?
> > > > 
> > > >  From my understanding, CLK_IGNORE_UNUSED will prevent the clock to be
> > > > gated during boot (on "clk: Disabling unused clocks"), but after it, the
> > > > clock can be gated.
> > > > 
> > > > With CLK_IS_CRITICAL, the clock will never be disabled.
> > > 
> > > Yeah, that's correct.
> > > 
> > > > For example, RPI_FIRMWARE_M2MC_CLK_ID is used by vc4. It needs to be
> > > > enabled at boot (I tested; if not enabled, it won't boot). However,
> > > > after vc4 is probed, we would like vc4 to have control of it and be able
> > > > to unprepare it in `vc4_hdmi_runtime_suspend()`. If I set it as
> > > > CLK_IS_CRITICAL, vc4 won't be able to unprepare it.
> > > 
> > > If the clock can be disabled by Linux, but it breaks some drivers if
> > > it's not enabled during their probe, something is fishy somewhere, and
> > > it's likely it would be just as broken if you compiled the driver as a
> > > module.
> > > 
> > > Even then, some of the other clocks should probably never be disabled,
> > > like the CPU clock.
> > 
> > I'll mark RPI_FIRMWARE_ARM_CLK_ID and RPI_FIRMWARE_CORE_CLK_ID as
> > critical. Are there any other clocks you think should never be disabled?
> > 
> > > 
> > > > I only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
> > > > RPi won't reboot.
> > > 
> > > Why?
> > 
> > I'll have to dig a bit into vc4 HDMI code and to investigate the reason
> > (and maybe fix the issue there).
> 
> After some investigation, I believe that those display-related should be
> set to CLK_IGNORE_UNUSED. It's not that it breaks some drivers if not
> enabled, but it breaks hardware functionality and the device won't boot.

That's the sign of a driver accessing a register without having its
clock explicitly enabled. It's a bug in the driver.

> See, for example, clk-bcm2835 in which all PLL and PLL dividers clocks
> are marked with CLK_IGNORE_UNUSED and some with CLK_IS_CRITICAL.
>
> Maybe Dave has some input about the topic?
> 
> So far, I'm planing to keep CLK_IGNORE_UNUSED to the display-related
> clocks and remove CLK_IS_CRITICAL from RPI_FIRMWARE_PIXEL_BVB_CLK_ID. If
> you have any objections about it, let me know.

I guess my main point was that CLK_IGNORE_UNUSED is inherently fragile
and doesn't protect from the clock being actually disabled: a driver
probing, failing and calling clk_disable on that clock would disable it
for example. Or if the last enabled sibling reparents to another parent
and thus your parent will get disabled.

The only thing it does is skipping the explicit disable round at the end
of the kernel boot. That's it.

So if you do need to keep the clock enabled, use CLK_IS_CRITICAL. If you
don't, well then that's easy :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]