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

[Maxime Ripard]

On Tue, Jul 29, 2025 at 09:33:50PM -0300, Maíra Canal wrote:
> 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.

That's the sign of a driver accessing a register without having its
clock explicitly enabled. It's a bug in the driver.

> 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.

I guess my main point was that CLK_IGNORE_UNUSED is inherently fragile
and doesn't protect from the clock being actually disabled: a driver
probing, failing and calling clk_disable on that clock would disable it
for example. Or if the last enabled sibling reparents to another parent
and thus your parent will get disabled.

The only thing it does is skipping the explicit disable round at the end
of the kernel boot. That's it.

So if you do need to keep the clock enabled, use CLK_IS_CRITICAL. If you
don't, well then that's easy :)

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250730/f2caa24d/attachment.sig>