On Mon, Feb 20, 2023 at 05:46:36PM +0200, Abel Vesa wrote: > On 23-02-17 21:38:22, Stephen Boyd wrote: > > Quoting Abel Vesa (2022-12-27 12:45:27) > > > There are unused clocks that need to remain untouched by clk_disable_unused, > > > and most likely could be disabled later on sync_state. So provide a generic > > > sync_state callback for the clock providers that register such clocks. > > > Then, use the same mechanism as clk_disable_unused from that generic > > > callback, but pass the device to make sure only the clocks belonging to > > > the current clock provider get disabled, if unused. Also, during the > > > default clk_disable_unused, if the driver that registered the clock has > > > the generic clk_sync_state_disable_unused callback set for sync_state, > > > skip disabling its clocks. > > > > How does that avoid disabling clks randomly in the clk tree? I'm > > concerned about disabling an unused clk in the middle of the tree > > because it doesn't have a driver using sync state, while the clk is the > > parent of an unused clk that is backed by sync state. > > > > clk A --> clk B > > > > clk A: No sync state > > clk B: sync state > > > > clk B is left on by the bootloader. __clk_disable_unused(NULL) is called > > from late init. Imagine clk A is the root of the tree. > > > > clk_disable_unused_subtree(clk_core A) > > clk_disable_unused_subtree(clk_core B) > > if (from_sync_state && core->dev != dev) > > return; > > ... > > clk core A->ops->disable() > > > > clk core B is off now? > > Yes, that is correct. But the same thing is happening currently if the > clk_ignore_unused in not specified. At least with this new approach, we > get to leave unused clocks enabled either until sync_state is called or forever. > All the provider has to do is to implement a sync_state callback (or use > the generic one provided). So the provider of clk A would obviously need > a sync state callback registered. > > > > > Also sync_state seems broken right now. I saw mka mentioned that if you > > have a device node enabled in your DT but never enable a driver for it > > in the kernel we'll never get sync_state called. This is another > > problem, but it concerns me that sync_state would make the unused clk > > disabling happen at some random time or not at all. > > Well, the fact that the sync state not being called because a driver for > a consumer device doesn't probe does not really mean it is broken. Just > because the consumer driver hasn't probed yet, doesn't mean it will > not probe later on. > > That aside, rather than going with clk_ignore_unused all the time on > qcom platforms, at least in a perfect scenario (where sync state is > reached for all providers) the clocks get disabled. > Furthermore, the sync_state approach will cause clk_disable_unused() to be invoked for clock drivers that probe after late_initcall() as well. So not only can we boot without clk_ignore_unused, we will actually turn off unused display clocks (perhaps all of them, for a headless device). Regards, Bjorn
