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

[Maíra Canal]

Hi Maxime,

On 05/03/26 05:53, Maxime Ripard wrote:
> On Wed, Feb 25, 2026 at 10:57:11AM -0300, Maíra Canal wrote:
>> Hi Maxime,
>>
>> On 24/02/26 11:11, Maxime Ripard wrote:
>>> Hi Maira,
>>>
>>

[...]

>>>> @@ -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.
> 
> For both of those, I still feel like it's a pretty strong deviation from
> the general expected behaviour of the CCF API. From what you're saying,
> we shouldn't notice it too much, but at the very least we should
> document it explicitly, both what the deviation is, and why we think
> it's ok.
> 

I'll make sure to add such comment in the next version.

>>> 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.
> 
> ack :)
> 
>>>
>>>>    	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.
> 
> min_rate itself is indeed available only in raspberrypi_clk_register(),
> but its main use is to call clk_hw_set_rate_range to use it as the
> minimum clock rate.
 > > In prepare/unprepare, you should be able to use clk_hw_get_rate_range()
> to retrieve it, or am I missing something?

Okay, I got now, I didn't know about the function
clk_hw_get_rate_range(). Thanks for your feedback! I'd address it in the
next version.

Best regards,
- Maíra

> 
> Maxime