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
