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

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

[Hans de Goede]

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.

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

Regards,

Hans


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.

3) There is precedence for something like this in other places in the
   kernel, e.g. pm_runtime_put_noidle() and the power_off parameter
   to dev_pm_domain_detach()

4) With the risk of the 2 copies of that logic diverging over time.



Hans de Goede (3):
  clk: Add __clk_disable_unprepare_counts_only() helper
  drm/sysfb: simpledrm: Use __clk_disable_unprepare_counts_only()
  fbdev: simplefb: Use __clk_disable_unprepare_counts_only()

 drivers/clk/clk.c                 | 41 ++++++++++++++++++++++---------
 drivers/gpu/drm/sysfb/simpledrm.c |  4 +--
 drivers/video/fbdev/simplefb.c    |  2 +-
 include/linux/clk.h               | 14 +++++++++++
 4 files changed, 46 insertions(+), 15 deletions(-)

-- 
2.54.0

[PATCH 1/3] clk: Add __clk_disable_unprepare_counts_only() helper

[Hans de Goede]

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 and the simpledrm driver calls
clk_prepare_enable() on these clocks. When the real display driver loads
simpledrm gets unbound and calls clk_disable_unprepare().

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.

Sometimes this leaves the hardware in an undefined state e.g. on some
Qualcomm platforms turning off the DP clocks at simpledrm remove() results
in 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

Clocks from the simple-framebuffer DT node are not so much clocks which
the simpledrm driver should take full control of, but rather are clocks
which should not be touched as long as the firmware framebuffer is in use.
This includes not turning them off before handing over control to the real
display driver.

Add a new __clk_disable_unprepare_counts_only() helper which decrements the
enable, prepare and protect counts of the clock and its parents, so that
the real display can take control over the clocks, but which does not
actually disable any clocks.

Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/clk/clk.c   | 41 +++++++++++++++++++++++++++++------------
 include/linux/clk.h | 14 ++++++++++++++
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 47093cda9df3..8c92d3551556 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1037,7 +1037,7 @@ int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(devm_clk_rate_exclusive_get);
 
-static void clk_core_unprepare(struct clk_core *core)
+static void clk_core_unprepare(struct clk_core *core, bool call_unprepare_op)
 {
 	lockdep_assert_held(&prepare_lock);
 
@@ -1062,18 +1062,18 @@ static void clk_core_unprepare(struct clk_core *core)
 
 	trace_clk_unprepare(core);
 
-	if (core->ops->unprepare)
+	if (call_unprepare_op && core->ops->unprepare)
 		core->ops->unprepare(core->hw);
 
 	trace_clk_unprepare_complete(core);
-	clk_core_unprepare(core->parent);
+	clk_core_unprepare(core->parent, call_unprepare_op);
 	clk_pm_runtime_put(core);
 }
 
 static void clk_core_unprepare_lock(struct clk_core *core)
 {
 	clk_prepare_lock();
-	clk_core_unprepare(core);
+	clk_core_unprepare(core, true);
 	clk_prepare_unlock();
 }
 
@@ -1140,7 +1140,7 @@ static int clk_core_prepare(struct clk_core *core)
 
 	return 0;
 unprepare:
-	clk_core_unprepare(core->parent);
+	clk_core_unprepare(core->parent, true);
 runtime_put:
 	clk_pm_runtime_put(core);
 	return ret;
@@ -1178,7 +1178,7 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
-static void clk_core_disable(struct clk_core *core)
+static void clk_core_disable(struct clk_core *core, bool call_disable_op)
 {
 	lockdep_assert_held(&enable_lock);
 
@@ -1197,12 +1197,12 @@ static void clk_core_disable(struct clk_core *core)
 
 	trace_clk_disable(core);
 
-	if (core->ops->disable)
+	if (call_disable_op && core->ops->disable)
 		core->ops->disable(core->hw);
 
 	trace_clk_disable_complete(core);
 
-	clk_core_disable(core->parent);
+	clk_core_disable(core->parent, call_disable_op);
 }
 
 static void clk_core_disable_lock(struct clk_core *core)
@@ -1210,7 +1210,7 @@ static void clk_core_disable_lock(struct clk_core *core)
 	unsigned long flags;
 
 	flags = clk_enable_lock();
-	clk_core_disable(core);
+	clk_core_disable(core, true);
 	clk_enable_unlock(flags);
 }
 
@@ -1235,6 +1235,23 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+void __clk_disable_unprepare_counts_only(struct clk *clk)
+{
+	unsigned long flags;
+
+	if (IS_ERR_OR_NULL(clk))
+		return;
+
+	flags = clk_enable_lock();
+	clk_core_disable(clk->core, false);
+	clk_enable_unlock(flags);
+
+	clk_prepare_lock();
+	clk_core_unprepare(clk->core, false);
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(__clk_disable_unprepare_counts_only);
+
 static int clk_core_enable(struct clk_core *core)
 {
 	int ret = 0;
@@ -1262,7 +1279,7 @@ static int clk_core_enable(struct clk_core *core)
 		trace_clk_enable_complete(core);
 
 		if (ret) {
-			clk_core_disable(core->parent);
+			clk_core_disable(core->parent, true);
 			return ret;
 		}
 	}
@@ -2451,7 +2468,7 @@ static void clk_change_rate(struct clk_core *core)
 
 	if (core->flags & CLK_SET_RATE_UNGATE) {
 		clk_core_disable_lock(core);
-		clk_core_unprepare(core);
+		clk_core_unprepare(core, true);
 	}
 
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
@@ -4063,7 +4080,7 @@ static int __clk_core_init(struct clk_core *core)
 		if (ret) {
 			pr_warn("%s: critical clk '%s' failed to enable\n",
 			       __func__, core->name);
-			clk_core_unprepare(core);
+			clk_core_unprepare(core, true);
 			goto out;
 		}
 	}
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 998ba3f261da..7dee0dbb9018 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -783,6 +783,19 @@ void clk_disable(struct clk *clk);
  */
 void clk_bulk_disable(int num_clks, const struct clk_bulk_data *clks);
 
+/**
+ * __clk_disable_unprepare_counts_only - Decrement enable and prepare counts
+ *
+ * Like clk_disable_unprepare() but then only decrement the enable, prepare and
+ * protect counts of the clock and its parents without actually disabling any
+ * clocks.
+ *
+ * This function should only be used in special case where the clocks should
+ * stay on while handing control over from an early-boot driver like simpledrm
+ * to a later more featureful driver. When in doubt do NOT use this function.
+ */
+void __clk_disable_unprepare_counts_only(struct clk *clk);
+
 /**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
@@ -1099,6 +1112,7 @@ static inline int __must_check clk_bulk_enable(int num_clks,
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline void __clk_disable_unprepare_counts_only(struct clk *clk) {}
 
 static inline void clk_bulk_disable(int num_clks,
 				    const struct clk_bulk_data *clks) {}
-- 
2.54.0

[PATCH 2/3] drm/sysfb: simpledrm: Use __clk_disable_unprepare_counts_only()

[Hans de Goede]

simpledrm_device_init_clocks() calls clk_prepare_enable() to avoid clocks
used by the framebuffer getting turned off while in use.

simpledrm_device_release_clocks() then must call clk_disable_unprepare()
to balance the enable count. 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 display power-off sequence.

Sometimes this leaves the hardware in an undefined state e.g. on some
Qualcomm platforms turning off the DP clocks at simpledrm remove() results
in 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

Switch to using the new __clk_disable_unprepare_counts_only() function,
which decrements the counts without actually turning off the clocks.

Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/gpu/drm/sysfb/simpledrm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 7a95d2dacd9d..6927a32ed282 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -261,7 +261,7 @@ static void simpledrm_device_release_clocks(void *res)
 
 	for (i = 0; i < sdev->clk_count; ++i) {
 		if (sdev->clks[i]) {
-			clk_disable_unprepare(sdev->clks[i]);
+			__clk_disable_unprepare_counts_only(sdev->clks[i]);
 			clk_put(sdev->clks[i]);
 		}
 	}
@@ -315,7 +315,7 @@ static int simpledrm_device_init_clocks(struct simpledrm_device *sdev)
 	while (i) {
 		--i;
 		if (sdev->clks[i]) {
-			clk_disable_unprepare(sdev->clks[i]);
+			__clk_disable_unprepare_counts_only(sdev->clks[i]);
 			clk_put(sdev->clks[i]);
 		}
 	}
-- 
2.54.0

[PATCH 3/3] fbdev: simplefb: Use __clk_disable_unprepare_counts_only()

[Hans de Goede]

simplefb_clocks_enable() calls clk_prepare_enable() to avoid clocks used by
the framebuffer getting turned off while in use.

This requires simplefb_clocks_destroy() calling clk_disable_unprepare() to
balance the enable count. 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 display power-off sequence.

Sometimes this leaves the hardware in an undefined state e.g. on some
Qualcomm platforms turning off the DP clocks at simplefb_destroy() results
in 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 work display output.

Switch to using the new __clk_disable_unprepare_counts_only() function,
which decrements the counts without actually turning off the clocks.

Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/video/fbdev/simplefb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 60e5dcec201f..b1e0dba6d127 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -303,7 +303,7 @@ static void simplefb_clocks_destroy(struct simplefb_par *par)
 	for (i = 0; i < par->clk_count; i++) {
 		if (par->clks[i]) {
 			if (par->clks_enabled)
-				clk_disable_unprepare(par->clks[i]);
+				__clk_disable_unprepare_counts_only(par->clks[i]);
 			clk_put(par->clks[i]);
 		}
 	}
-- 
2.54.0

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

[Maxime Ripard]

[-- 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 --]

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

[Hans de Goede]

Hi,

On 27-May-26 14:04, Maxime Ripard wrote:
> 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.

That depends on how the clks are configured / setup by the clk driver
if the CLK_SET_RATE_GATE is set in clk_core.flags then protect_count
will get incremented and from then on the rate will be locked.

We could even have the simpledrm / simplefb code call 
clk_rate_exclusive_get() to lock the rate.

So there are multiple ways to lock the rate if necessary but so far
in the real world there has been no need for this AFAICT.

So IMHO this is something to worry about if / when we hit this
actually happening in a real world scenario.

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

Generally speaking there is a contract with the firmware that either
the clock is firmware controlled and the OS can maybe make firmware calls
to request changes; or the OS is in control. Both trying to control
the same clk at the same time is a bug and IMHO unrelated to the issue
at hand which is about clks which are under OS control only.

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

See above, this is exactly what the protect_count in the clk-core
is for.

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

That is certainly an interesting proposal but IMHO almost orthogonal
to this one (1). Even if it may fix the issue in the end, it seems
that that work is still quite a way from going upstream and even then
initially it only potentially fixes this for the TIDSS driver since
that solution needs a lot of per driver work.

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

So I would still like to move forward with the fix proposed here.

Regards,

Hans


1) As you write yourself:

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

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

[Maxime Ripard]

[-- 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 --]

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

[Hans de Goede]

Hi Maxime,

On 27-May-26 14:48, Maxime Ripard wrote:
> 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.

AFAICT in its current version it does not contain a mechanism to
delay the remove conflicting framebuffer call, so no it does not
fix my 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?

I would prefer my proposed KISS solution, which works everywhere, over
your quite complex solution which requires complex per driver changes.

When I was still working on:

https://fedoraproject.org/wiki/Changes/FlickerFreeBoot

I needed to constantly fix i915 fastset support and when I stopped
fixing it due to -ENOTIME it pretty much was broken most of the time.

Last time I check modern (Alderlake +) laptops never had working
fastset support and there always is a visible most set on newer
laptop models because of this.

This state-readback stuff is quite complicated, as the list of
known issues in your cover-letter shows. I wish you all the best
with it, but I really do not see this as a good alternative for
fixing the issues *this* series tries to address given the huge
disparity in complexity between the 2 fixes.

I'm also worried that the readback stuff is likely going to be fragile
so might end up being disabled or automatically detect some issue and
disable itself at which point we're back to having the original
problem this series tries to fix.

Also atm AFAIK there are no plans to add state readback support to
msm and there are a lot of other higher priority items to work on.

###

Anyways since your patch-series will not work for msm in its
current state, I would still very much like to hear from the clock
maintainers what they think of the approach suggested here.

Regards,

Hans

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

[Maxime Ripard]

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

On Wed, May 27, 2026 at 05:09:39PM +0200, Hans de Goede wrote:
> Hi Maxime,
> 
> On 27-May-26 14:48, Maxime Ripard wrote:
> > 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.
> 
> AFAICT in its current version it does not contain a mechanism to
> delay the remove conflicting framebuffer call, so no it does not
> fix my problem.

The conflicting framebuffer is removed by drivers through an explicit
call to aperture_remove_all_conflicting_devices(). Just move that call
after the call to drm_mode_config_create_initial_state, there's nothing
to integrate.

> >> 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?
> 
> I would prefer my proposed KISS solution, which works everywhere, over
> your quite complex solution which requires complex per driver changes.
> 
> When I was still working on:
> 
> https://fedoraproject.org/wiki/Changes/FlickerFreeBoot
> 
> I needed to constantly fix i915 fastset support and when I stopped
> fixing it due to -ENOTIME it pretty much was broken most of the time.
> 
> Last time I check modern (Alderlake +) laptops never had working
> fastset support and there always is a visible most set on newer
> laptop models because of this.
> 
> This state-readback stuff is quite complicated, as the list of
> known issues in your cover-letter shows.

It's kind of a bad faith argument. This list is 4 issues. One is getting
fixed in the next version, the other two are specific to tidss which you
don't care about.

> I wish you all the best with it, but I really do not see this as a
> good alternative for fixing the issues *this* series tries to address
> given the huge disparity in complexity between the 2 fixes.
> 
> I'm also worried that the readback stuff is likely going to be fragile
> so might end up being disabled or automatically detect some issue and
> disable itself at which point we're back to having the original
> problem this series tries to fix.
> 
> Also atm AFAIK there are no plans to add state readback support to
> msm and there are a lot of other higher priority items to work on.
> 
> ###
> 
> Anyways since your patch-series will not work for msm in its
> current state, I would still very much like to hear from the clock
> maintainers what they think of the approach suggested here.

I really don't get this argument either. Sure, it doesn't work for msm.
But the clock framework didn't have that unsafe function call either,
and yet you did the work to add it.

Maxime

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

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

[Brian Masney]

Hi Hans,

On Wed, May 27, 2026 at 11:48:08AM +0200, Hans de Goede wrote:
> 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.

Why won't that work in the case where different parent clocks are selected?
I'll describe a scenario below.

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

I think the deferred unbinding could have some potential here where
there is some kind of notification mechanism between simple-framebuffer
and the real drm driver. So:

- simple-framebuffer driver takes reference(s) to the clk(s).

- real drm driver eventually loads, takes reference(s) to the necessary
  clk(s).

- real drm driver sends a notification to simple-framebuffer that it's
  done, and has control.

- simple-framebuffer can unbind and release its references to the clks.

No clks will be shutdown prematurely in this scenario.

If the real drm driver needs a different parent, then presumably things
should be setup correctly, and simple-framebuffer can have the clocks
shut down when it calls clk_disable_unprepare(). If the real drm driver
needed those clks, then it should hold a reference to them.

I'm intentionally not going through how to do the notification mechanism
here.

Brian

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

[Hans de Goede]

Hi Brian,

On 28-May-26 1:03 AM, Brian Masney wrote:
> Hi Hans,
> 
> On Wed, May 27, 2026 at 11:48:08AM +0200, Hans de Goede wrote:
>> 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.
> 
> Why won't that work in the case where different parent clocks are selected?
> I'll describe a scenario below.
> 
>>
>>    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.
> 
> I think the deferred unbinding could have some potential here where
> there is some kind of notification mechanism between simple-framebuffer
> and the real drm driver. So:
> 
> - simple-framebuffer driver takes reference(s) to the clk(s).
> 
> - real drm driver eventually loads, takes reference(s) to the necessary
>   clk(s).
> 
> - real drm driver sends a notification to simple-framebuffer that it's
>   done, and has control.
> 
> - simple-framebuffer can unbind and release its references to the clks.
> 
> No clks will be shutdown prematurely in this scenario.
> 
> If the real drm driver needs a different parent, then presumably things
> should be setup correctly, and simple-framebuffer can have the clocks
> shut down when it calls clk_disable_unprepare().

If the real drm driver needs a different parent, then how does it
do the reparenting while the simple-framebuffer driver is holding
a reference to the clock ?  In that case the clock might have
a protected_count of non 0 (depends on the core-clk flags) and
reparenting won't work.

We could have the drm driver claim the clks, then call some release
function and only then do the reparenting. But this makes things
somewhat complicated for the drm drivers.

So my conclusion is that having a clk equivalent of e.g.
pm_runtime_put_noidle() which just lowers the counts without
actually doing anything is still the most KISS approach here.

With the downside of needing a new clk-core function for this.

> If the real drm driver
> needed those clks, then it should hold a reference to them.
>
> I'm intentionally not going through how to do the notification mechanism> here.

Ack.

Regards,

Hans

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

[Maxime Ripard]

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

On Thu, May 28, 2026 at 12:12:10PM +0200, Hans de Goede wrote:
> Hi Brian,
> 
> On 28-May-26 1:03 AM, Brian Masney wrote:
> > Hi Hans,
> > 
> > On Wed, May 27, 2026 at 11:48:08AM +0200, Hans de Goede wrote:
> >> 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.
> > 
> > Why won't that work in the case where different parent clocks are selected?
> > I'll describe a scenario below.
> > 
> >>
> >>    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.
> > 
> > I think the deferred unbinding could have some potential here where
> > there is some kind of notification mechanism between simple-framebuffer
> > and the real drm driver. So:
> > 
> > - simple-framebuffer driver takes reference(s) to the clk(s).
> > 
> > - real drm driver eventually loads, takes reference(s) to the necessary
> >   clk(s).
> > 
> > - real drm driver sends a notification to simple-framebuffer that it's
> >   done, and has control.
> > 
> > - simple-framebuffer can unbind and release its references to the clks.
> > 
> > No clks will be shutdown prematurely in this scenario.
> > 
> > If the real drm driver needs a different parent, then presumably things
> > should be setup correctly, and simple-framebuffer can have the clocks
> > shut down when it calls clk_disable_unprepare().
> 
> If the real drm driver needs a different parent, then how does it
> do the reparenting while the simple-framebuffer driver is holding
> a reference to the clock ?  In that case the clock might have
> a protected_count of non 0 (depends on the core-clk flags) and
> reparenting won't work.

The only case where it should reparent you listed was that you might
need to pick up a different resolution. However, that can only be
enforced by an ioctl or a client.

simplefb/drm is removed in msm_drm_kms_init. The device is published
drm_dev_register called right after msm_drm_kms_init, and the clients
are registered in msm_drm_kms_post_init, called after drm_dev_register.

There's no way in the current msm architecture to have a modeset happen
while the simpledrm driver is still active.

Maxime

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

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

[Hans de Goede]

Hi,

On 28-May-26 2:08 PM, Maxime Ripard wrote:
> On Thu, May 28, 2026 at 12:12:10PM +0200, Hans de Goede wrote:
>> Hi Brian,
>>
>> On 28-May-26 1:03 AM, Brian Masney wrote:
>>> Hi Hans,
>>>
>>> On Wed, May 27, 2026 at 11:48:08AM +0200, Hans de Goede wrote:
>>>> 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.
>>>
>>> Why won't that work in the case where different parent clocks are selected?
>>> I'll describe a scenario below.
>>>
>>>>
>>>>    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.
>>>
>>> I think the deferred unbinding could have some potential here where
>>> there is some kind of notification mechanism between simple-framebuffer
>>> and the real drm driver. So:
>>>
>>> - simple-framebuffer driver takes reference(s) to the clk(s).
>>>
>>> - real drm driver eventually loads, takes reference(s) to the necessary
>>>   clk(s).
>>>
>>> - real drm driver sends a notification to simple-framebuffer that it's
>>>   done, and has control.
>>>
>>> - simple-framebuffer can unbind and release its references to the clks.
>>>
>>> No clks will be shutdown prematurely in this scenario.
>>>
>>> If the real drm driver needs a different parent, then presumably things
>>> should be setup correctly, and simple-framebuffer can have the clocks
>>> shut down when it calls clk_disable_unprepare().
>>
>> If the real drm driver needs a different parent, then how does it
>> do the reparenting while the simple-framebuffer driver is holding
>> a reference to the clock ?  In that case the clock might have
>> a protected_count of non 0 (depends on the core-clk flags) and
>> reparenting won't work.
> 
> The only case where it should reparent you listed was that you might
> need to pick up a different resolution. However, that can only be
> enforced by an ioctl or a client.
> 
> simplefb/drm is removed in msm_drm_kms_init. The device is published
> drm_dev_register called right after msm_drm_kms_init, and the clients
> are registered in msm_drm_kms_post_init, called after drm_dev_register.
> 
> There's no way in the current msm architecture to have a modeset happen
> while the simpledrm driver is still active.

Ok, new plan, please let me know what you think about this:

1. Add a new "disable" callback argument to
devm_aperture_acquire_for_platform_device() and store this in
struct aperture_range

2. Add a new aperture_disable_conflicting_devices() which
calls the disable callback for matching devices.

3. Have the simple[fd|drm] drivers implement a disable callback
which unregisters the drm dev and releases any claims on the
aperture mem-region, while keeping clks, regulators, etc.
enabled. And have them check if disable was called on remove()
and if not do the disable() things on remove(0

4. Have msm call aperture_disable_conflicting_devices() where it
now call aperture_remove_conflicting_devices() and call
aperture_remove_conflicting_devices() at the point where it has
claimed any clks it needs.

Does this sound like something which would be acceptable ?

Regards,

Hans