Re: [PATCH 1/3] clk: bcm: rpi: Turn firmware clock on/off when preparing/unpreparing

["Maíra Canal" <mcanal@igalia.com>]

Hi Stefan,

On 28/07/25 13:33, Stefan Wahren wrote:
> Hi Maíra,
> 
> thanks for working on this.
> 
> Am 28.07.25 um 14:35 schrieb Maíra Canal:
>> Currently, when we prepare or unprepare RPi's clocks, we don't actually
>> enable/disable the firmware clock. This means that
>> `clk_disable_unprepare()` doesn't actually change the clock state at
>> all, nor does it lowers the clock rate.
>>
>>  From the Mailbox Property Interface documentation [1], we can see that
>> we should use `RPI_FIRMWARE_SET_CLOCK_STATE` to set the clock state
>> off/on. Therefore, use `RPI_FIRMWARE_SET_CLOCK_STATE` to create a
>> prepare and an unprepare hook for RPi's firmware clock.
>>
>> As now the clocks are actually turned off, some of them are now marked
>> with CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, as those are required since
>> early boot or are required during reboot.
>>
>> Link: https://github.com/raspberrypi/firmware/wiki/Mailbox-property- 
>> interface [1]
>> Fixes: 93d2725affd6 ("clk: bcm: rpi: Discover the firmware clocks")
> could you please explain from user perspective, which issue is fixed by 
> this patch?

I was about to talk about the power savings benefits for the user.
However, as I type, I notice that such a thing doesn't justify a
"Fixes:" tag. I'll drop it.

Thanks for your review, I'll address all the comments.

Best Regards,
- Maíra

> 
> Why does this needs to be backported?
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/clk/bcm/clk-raspberrypi.c | 41 +++++++++++++++++++++++++++++ 
>> +++++++++-
>>   1 file changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk- 
>> raspberrypi.c
>> index 
>> 8e4fde03ed232b464165f524d27744b4ced93a60..a2bd5040283a2f456760bd685e696b423985cac0 100644
>> --- a/drivers/clk/bcm/clk-raspberrypi.c
>> +++ b/drivers/clk/bcm/clk-raspberrypi.c
>> @@ -68,6 +68,7 @@ struct raspberrypi_clk_variant {
>>       char        *clkdev;
>>       unsigned long    min_rate;
>>       bool        minimize;
>> +    u32        flags;
>>   };
>>   static struct raspberrypi_clk_variant
>> @@ -75,6 +76,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>       [RPI_FIRMWARE_ARM_CLK_ID] = {
>>           .export = true,
>>           .clkdev = "cpu0",
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_CORE_CLK_ID] = {
>>           .export = true,
>> @@ -90,6 +92,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>            * always use the minimum the drivers will let us.
>>            */
>>           .minimize = true,
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_M2MC_CLK_ID] = {
>>           .export = true,
>> @@ -115,6 +118,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>            * drivers will let us.
>>            */
>>           .minimize = true,
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_V3D_CLK_ID] = {
>>           .export = true,
>> @@ -127,6 +131,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>       [RPI_FIRMWARE_HEVC_CLK_ID] = {
>>           .export = true,
>>           .minimize = true,
>> +        .flags = CLK_IGNORE_UNUSED,
>>       },
>>       [RPI_FIRMWARE_ISP_CLK_ID] = {
>>           .export = true,
>> @@ -135,6 +140,7 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = {
>>       [RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = {
>>           .export = true,
>>           .minimize = true,
>> +        .flags = CLK_IS_CRITICAL,
>>       },
>>       [RPI_FIRMWARE_VEC_CLK_ID] = {
>>           .export = true,
>> @@ -259,7 +265,40 @@ static int 
>> raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw,
>>       return 0;
>>   }
>> +static int raspberrypi_fw_prepare(struct clk_hw *hw)
>> +{
>> +    const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
>> +    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)
>> +        dev_err(rpi->dev, "Failed to set clock %d state to on: %d",
>> +            data->id, ret);
> I suggest to use dev_err_ratelimited for prepare/unprepare, otherwise 
> this could spam the kernel log.
> 
> Furthermore i wouldn't recommend to log some magic clock id. How about 
> using clk_hw_get_name(hw) instead?
> 
> Don't we need a newline character at the end?
> 
>> +
>> +    return ret;
>> +}
>> +
>> +static void raspberrypi_fw_unprepare(struct clk_hw *hw)
>> +{
>> +    const struct raspberrypi_clk_data *data = clk_hw_to_data(hw);
>> +    struct raspberrypi_clk *rpi = data->rpi;
>> +    u32 state = 0;
>> +    int ret;
>> +
>> +    ret = raspberrypi_clock_property(rpi->firmware, data,
>> +                     RPI_FIRMWARE_SET_CLOCK_STATE, &state);
>> +    if (ret)
>> +        dev_err(rpi->dev, "Failed to set clock %d state to off: %d",
>> +            data->id, ret);
> see above
> 
> Best regards
>> +}
>> +
>> +
>>   static const struct clk_ops raspberrypi_firmware_clk_ops = {
>> +    .prepare        = raspberrypi_fw_prepare,
>> +    .unprepare      = raspberrypi_fw_unprepare,
>>       .is_prepared    = raspberrypi_fw_is_prepared,
>>       .recalc_rate    = raspberrypi_fw_get_rate,
>>       .determine_rate    = raspberrypi_fw_dumb_determine_rate,
>> @@ -289,7 +328,7 @@ static struct clk_hw 
>> *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
>>       if (!init.name)
>>           return ERR_PTR(-ENOMEM);
>>       init.ops = &raspberrypi_firmware_clk_ops;
>> -    init.flags = CLK_GET_RATE_NOCACHE;
>> +    init.flags = variant->flags | CLK_GET_RATE_NOCACHE;
>>       data->hw.init = &init;
>>
>