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

[Stefan Wahren <wahrenst@gmx.net>]

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