Date:   Mon, 20 Feb 2023 08:33:54 -0800
From:   Bjorn Andersson <andersson@kernel.org>
To:     Abel Vesa <abel.vesa@linaro.org>
Cc:     Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Mike Turquette <mturquette@baylibre.com>,
        linux-clk@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org, mka@chromium.org
Subject: Re: [PATCH v3 1/2] clk: Add generic sync_state callback for
 disabling unused clocks
Message-ID: <20230220163354.keppkzdt6nzh6xfb@ripper>
References: <20221227204528.1899863-1-abel.vesa@linaro.org>
 <ebc257025ebd641e624ef506ea09c800.sboyd@kernel.org>
 <Y/OV3CF0ootyooDJ@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y/OV3CF0ootyooDJ@linaro.org>
Precedence: bulk

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