Re: [PATCH 0/3] clk: Add __clk_disable_unprepare_counts_only() and use this in simple[fb|drm]

[Maxime Ripard <mripard@kernel.org>]

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

Hi Hans,

On Wed, May 27, 2026 at 11:48:08AM +0200, Hans de Goede wrote:
> Hi Michael, Stephen, et al.,
> 
> The device-tree simple-framebuffer device is special in that it does not
> describe actual hardware, but it allows firmware (e.g. u-boot) to pass a
> firmware initialized framebuffer to the OS to use as display during boot.
> 
> To avoid clocks used by the firmware framebuffer getting touched while in
> use, the DT node contains a list of clocks which should not be touched
> while the simple-framebuffer is in use.
> 
> Conceptually this is fine, in practice the *do not touch* is implemented as
> a clk_prepare_enable() + clk_disable_unprepare() pair, with the disabling
> happening when the simple-framebuffer driver gets replaced by the real
> display-engine driver.

Which is also pretty fragile because it doesn't lock the rate either.

> The simple-framebuffer concept assumes that the order in which resources
> are acquired does not matter since they are already on, so power-sequencing
> is not an issue. But clk_disable_unprepare() actually turns the clock off,
> messing with the hardware state before the real display driver loads,
> without any knowledge of the proper power-off sequence for the display.
> 
> The turning off of the clocks outside of a proper power-off sequence causes
> the following error when the msm display driver tries to re-enable them:
> 
> [    2.980181] disp_cc_mdss_dptx3_pixel0_clk_src: rcg didn't update its configuration.
> [    2.980272] WARNING: drivers/clk/qcom/clk-rcg2.c:136 at update_config+0xdc/0x100
> 
> Resulting in a non working display.
> 
> To fix this, we need a way to properly implement the "do not touch" concept
> for clocks.

I'm not sure it's something we can enforce, really. The firmware can
modify clocks behind our back. Other clocks might change this one as a
side-effect of a rate change or reparenting, etc. It's just not
something we can commit to.

> clk_prepare_enable() is fine, the clocks are already on so no
> ordering worries and it ensures that the clocks and their parents cannot
> get turned off by incrementing their enable, prepare and protect counts.
> 
> clk_disable_unprepare() is a problem though since it actually turns the
> clocks off. Instead something is needed which only decrements those counts.
> 
> This series introduces a new clk-core function called
> __clk_disable_unprepare_counts_only() (1) which does just that. Prefixed
> with '__' to indicate that normally drivers should not use this.
> 
> Michael, Stephen sorry for needing to add a new clk-core function for this,
> but I see no other way of solving this (2)(3). The changes are not that
> big / not too bad.
> 
> I've also considered making __clk_disable_unprepare_counts_only() implement
> all the logic itself instead of adding the extra parameter to
> clk_core_unprepare() and clk_core_disable() but that leads in duplicating
> quite a bit of logic (4) so this seems better.
> 
> The other 2 patches just replace the clk_disable_unprepare() calls in
> the simple[fb|drm] driver with the new helper.
> 
> This series fixes the display not coming up after switching to the msm
> driver when a simple-framebuffer node with clocks listed is used.
>
> 1) I'm open to changing the name, this is the best I could come up with.
> 
> 2) One option considered was detaching the simple-framebuffer driver later,
>    after the real display driver has had a chance to claim the clocks. But
>    this won't work in cases where the real display driver picks different
>    parent clocks then the boot firmware did and needs to reparent clocks.
> 
>    Basically the goal is for things to behave as if the simple-framebuffer
>    driver was not there at all, because that leaves the hw in the state
>    the real display driver expects.

So I know it's not really what you had in mind, but if you are only
concerned about the transition from the bootloader to the DRM driver,
then I think supporting the following work would help:

https://lore.kernel.org/r/20260423-drm-state-readout-v2-0-8549f87cb978@kernel.org

With that series, we can build the initial KMS state from the hardware
state, which means that if you were to change the resolution at boot, it
would be executed just like any mode change in KMS.

The initial state allocation is now fallible as well, so if you remove
simple-framebuffer after drm_mode_config_create_initial_state has been
called and has succeeded, then you know that the driver is now in
charge, picked up the configuration, and has taken all the resources
needed to continue running. And if it fails for any reason,
simple-framebuffer is still functional.

This should help mitigate your initial problem, even though it's a
widely different solution.

Maxime

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