Re: [PATCH v3 1/2] clk: Add generic sync_state callback for disabling unused clocks

[Saravana Kannan]

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