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: 3798 bytes --]

On Wed, May 27, 2026 at 02:21:50PM +0200, Hans de Goede wrote:
> >> 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.
> 
> Hmm, so your suggestion would be to have the initial KMS driver probe()
> only do a read back without touching anything. Then claim clks matching
> the read back config and then only release the simple* driver and thus
> the clocks after this?

Almost. Part of the call to drm_mode_config_create_initial_state() if
needed to make it grab its resources. See atomic_install_state in that
series. So probe wouldn't have anything more to do.

> That is certainly an interesting proposal but IMHO almost orthogonal
> to this one (1).

Why do you think it's orthogonal? It would completely fix your problem.

> Even if it may fix the issue in the end, it seems that that work is
> still quite a way from going upstream

It's likely to be merged in the next few weeks.

> and even then initially it only potentially fixes this for the TIDSS
> driver since that solution needs a lot of per driver work.

I mean, yeah, but the good thing is that you only have one driver to
care about, right?

> Where as my proposed fix here is much simpler and fixes the issue
> for all drivers in one go.

At the expense of exposing an unsafe function for only a single user and
usecase.

Maxime

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