On Thu, May 11, 2023 at 5:58 AM Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > On 23-02-21 11:58:24, Saravana Kannan wrote: > > On Mon, Feb 20, 2023 at 10:47 AM Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > > > > > On 23-02-20 09:51:55, Saravana Kannan wrote: > > > > On Mon, Feb 20, 2023 at 8:28 AM Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > > > > > > > > > On 23-02-20 17:46:36, 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. > > > > > > > > Hi Abel, > > > > > > > > We have the day off today, so I'll respond more later. Also, please cc > > > > me on all sync_state() related patches in the future. > > > > > > > > > > Sure thing. > > > > > > > I haven't taken a close look at your series yet, but at a glance it > > > > seems incomplete. > > > > > > > > Any reason you didn't just try to revive my series[1] or nudge me? > > > > [1]- https://lore.kernel.org/lkml/20210407034456.516204-3-saravanak@xxxxxxxxxx/ > > > > > > This patchset is heavily reworked and much more simpler as it relies > > > strictly on the sync_state being registered by the clock provider. > > > > It's simpler because it's not complete. It for sure doesn't handle > > orphan-reparenting. It also doesn't make a lot of sense for only some > > clock providers registering for sync_state(). If CC-A is feeding a > > clock signal that's used as a root for clocks in CC-B, then what > > happens if only CC-B implements sync_state() but CC-A doesn't. The > > clocks from CC-B are still going to turn off when CC-A turns off its > > PLL before CC-B registers. > > I gave your patchset a try and it breaks the uart for qcom platforms. > That is because your patchset enables the clock on __clk_core_init and > does not take into account the fact that 'boot enabled' clocks should be > left untouched. Those are probably just hacks when we didn't have sync_state(). But sure, we can make sure existing drivers aren't broken if the flag is set. > This also means the orphan-reparenting enabling should > be dropped as well. No, maybe for boot enabled clocks, but not for all clocks in general. You need this for sync_state() to work correctly for clocks left on at boot but "boot enabled" isn't set. > As for the second part, related to providers that might not have a > registered sync_state(), your patchset sets the clock core generic > one. This is also wrong because it doesn't take into account the fact > that there might be providers that need to do their own stuff on > sync_state() and should do that by registering their own implementation > of it. Right, in which case, they can set theirs or they get the default one. > Therefore, I'll respin your patchset and use only the skipping of > disabling the unused clocks, but I'll drop all the enable on init and orphan > reparenting changes. I think it'll result in a broken patch. Sorry, I've been a bit busy with some other work and I haven't been able to get to the clk_sync_state(). I'll try to rebase it soon and send it out too. -Saravana
