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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

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 only set RPI_FIRMWARE_PIXEL_BVB_CLK_ID as critical, as, otherwise, the
> RPi won't reboot.

Why?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]