Re: [PATCH v6 1/6] clk: bcm: rpi: Manage clock rate in prepare/unprepare callbacks

[Maxime Ripard <mripard@kernel.org>]

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

Hi Maira,

On Wed, Feb 18, 2026 at 05:44:59PM -0300, Maíra Canal wrote:
> On current firmware versions, RPI_FIRMWARE_SET_CLOCK_STATE doesn't
> actually power off the clock. To achieve meaningful power savings, the
> clock rate must be set to the minimum before disabling. This might be
> fixed in future firmware releases.
> 
> Rather than pushing rate management to clock consumers, handle it
> directly in the clock framework's prepare/unprepare callbacks. In
> unprepare, set the rate to the firmware-reported minimum before
> disabling the clock. In prepare, for clocks marked with `maximize`
> (currently v3d), restore the rate to the maximum after enabling.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/clk/bcm/clk-raspberrypi.c | 61 ++++++++++++++++++++++-----------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index 1a9162f0ae31e330c46f6eafdd00350599b0eede..0d6e4f43c3bac0e7b38934c5c6e4db51211233de 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -66,7 +66,8 @@ const struct raspberrypi_clk_data *clk_hw_to_data(const struct clk_hw *hw)
>  struct raspberrypi_clk_variant {
>  	bool		export;
>  	char		*clkdev;
> -	unsigned long	min_rate;
> +	u32		min_rate;
> +	u32		max_rate;
>  	bool		minimize;
>  	bool		maximize;
>  	u32		flags;
> @@ -289,16 +290,22 @@ static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
>  static int raspberrypi_fw_prepare(struct clk_hw *hw)
>  {
>  	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
> +	struct raspberrypi_clk_variant *variant = data->variant;
>  	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)
> +	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;
> +	}
> +
> +	if (variant->maximize)
> +		ret = raspberrypi_fw_set_rate(hw, variant->max_rate, 0);


It's not clear to me why you would want to force it to the max there,
and especially the max of the clock. It would make more sense to me to
set it to whatever maximum rate clk_hw_get_rate_range would return
(which should be the minimum of variant->max_rate + all clk->max_rate),
but even then it would erase every call to clk_set_rate before calling
clk_prepare().

I'd understand lowering the clock rate in unprepare to avoid wasting too
much power, but why do we need to raise it here?

>  	return ret;
>  }
> @@ -306,10 +313,19 @@ static int raspberrypi_fw_prepare(struct clk_hw *hw)
>  static void raspberrypi_fw_unprepare(struct clk_hw *hw)
>  {
>  	const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
> +	struct raspberrypi_clk_variant *variant = data->variant;
>  	struct raspberrypi_clk *rpi = data->rpi;
>  	u32 state = 0;
>  	int ret;
>  
> +	/*
> +	 * On current firmware versions, RPI_FIRMWARE_SET_CLOCK_STATE doesn't
> +	 * actually power off the clock. To achieve meaningful power consumption
> +	 * reduction, the driver needs to set the clock rate to minimum before
> +	 * disabling it.
> +	 */
> +	raspberrypi_fw_set_rate(hw, variant->min_rate, 0);

I'm not sure setting it to variant->min_rate would work directly either,
since it would break consumers expectations. Also, can we guard it with
a version check if we know that there's some good and bad firmwares?

>  	ret = raspberrypi_clock_property(rpi->firmware, data,
>  					 RPI_FIRMWARE_SET_CLOCK_STATE, &state);
>  	if (ret)
> @@ -334,7 +350,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
>  {
>  	struct raspberrypi_clk_data *data;
>  	struct clk_init_data init = {};
> -	u32 min_rate, max_rate;
> +	unsigned long rate;
>  	int ret;
>  
>  	data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL);
> @@ -354,18 +370,20 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
>  
>  	data->hw.init = &init;
>  
> -	ret = raspberrypi_clock_property(rpi->firmware, data,
> -					 RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
> -					 &min_rate);
> -	if (ret) {
> -		dev_err(rpi->dev, "Failed to get clock %d min freq: %d\n",
> -			id, ret);
> -		return ERR_PTR(ret);
> +	if (!variant->min_rate) {
> +		ret = raspberrypi_clock_property(rpi->firmware, data,
> +						 RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
> +						 &variant->min_rate);
> +		if (ret) {
> +			dev_err(rpi->dev, "Failed to get clock %d min freq: %d\n",
> +				id, ret);
> +			return ERR_PTR(ret);
> +		}
>  	}

It feels to me like it would need to be a separate patch. Why do you
need to override the minimum clock rate reported by the firmware?

Maxime

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