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

[Maíra Canal]

Hi Maxime,

On 24/02/26 11:11, Maxime Ripard wrote:
> Hi Maira,
> 

[...]

>> @@ -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?

This only applies to clocks with variant->maximize == true, which is
exclusively the V3D clock. The V3D driver doesn't do fine-grained rate
control, it always wants maximum performance. In the case of V3D, there
are no intermediate clk_set_rate() calls to preserve.

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

The clock is being unprepared, which means that any consumer that wants
to use it again must call clk_prepare() first, at which point the rate
gets restored (for maximize clocks) or re-established by the consumer
via clk_set_rate(). Considering that no consumer should be relying on
the rate of an unprepared clock, I understand that there are no
expectations to break here.

> a version check if we know that there's some good and bad firmwares?

So far, all firmware versions have this issue. For future firmware
releases, it might change, but so far, all firmware versions have this
issue.

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

This change is needed because the prepare/unprepare callbacks need
access to the min/max rates. In the current code, these are local
variables in raspberrypi_clk_register(). Storing them in the variant
struct makes them available to the callbacks. The `if (!variant-
 >min_rate)` guard only preserves the existing behavior for clocks like
M2MC that have a hard-coded minimum rate.

Having said that, I can split this into a separate preparatory patch if
you prefer.

Thanks for your review!

Best regards,
- Maíra

> 
> Maxime