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 <mca...@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;