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

Stefan Wahren
2025-08-18T10:52:30Z

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