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

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

Hi Maxime,

On 29/07/25 09:14, Maxime Ripard wrote:
> On Tue, Jul 29, 2025 at 08:53:51AM -0300, Maíra Canal wrote:
>> Hi Maxime,
>>
>> On 29/07/25 04:27, Maxime Ripard wrote:
>>> Hi Maíra,
>>>
>>> On Mon, Jul 28, 2025 at 09:35:38AM -0300, Maíra Canal wrote:
>>>> 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.
>>>
>>> What difference is there between the CLK_IGNORE_UNUSED and
>>> CLK_IS_CRITICAL clocks?
>>
>>  From my understanding, CLK_IGNORE_UNUSED will prevent the clock to be
>> gated during boot (on "clk: Disabling unused clocks"), but after it, the
>> clock can be gated.
>>
>> With CLK_IS_CRITICAL, the clock will never be disabled.
> 
> Yeah, that's correct.
> 
>> For example, RPI_FIRMWARE_M2MC_CLK_ID is used by vc4. It needs to be
>> enabled at boot (I tested; if not enabled, it won't boot). However,
>> after vc4 is probed, we would like vc4 to have control of it and be able
>> to unprepare it in `vc4_hdmi_runtime_suspend()`. If I set it as
>> CLK_IS_CRITICAL, vc4 won't be able to unprepare it.
> 
> If the clock can be disabled by Linux, but it breaks some drivers if
> it's not enabled during their probe, something is fishy somewhere, and
> it's likely it would be just as broken if you compiled the driver as a
> module.
> 
> Even then, some of the other clocks should probably never be disabled,
> like the CPU clock.

I'll mark RPI_FIRMWARE_ARM_CLK_ID and RPI_FIRMWARE_CORE_CLK_ID as
critical. Are there any other clocks you think should never be disabled?

> 
>> I only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
>> RPi won't reboot.
> 
> Why?

I'll have to dig a bit into vc4 HDMI code and to investigate the reason
(and maybe fix the issue there).

Best Regards,
- Maíra