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

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

On 29/07/25 13:19, Maíra Canal wrote:
> 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).

After some investigation, I believe that those display-related should be
set to CLK_IGNORE_UNUSED. It's not that it breaks some drivers if not
enabled, but it breaks hardware functionality and the device won't boot.
See, for example, clk-bcm2835 in which all PLL and PLL dividers clocks
are marked with CLK_IGNORE_UNUSED and some with CLK_IS_CRITICAL.

Maybe Dave has some input about the topic?

So far, I'm planing to keep CLK_IGNORE_UNUSED to the display-related
clocks and remove CLK_IS_CRITICAL from RPI_FIRMWARE_PIXEL_BVB_CLK_ID. If
you have any objections about it, let me know.

Best Regards,
- Maíra

> 
> Best Regards,
> - Maíra
>