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,
> >
>
> [...]
>
> > > @@ -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.
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.
> > 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?
Maxime
signature.asc
Description: PGP signature
