Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Jul 2023 09:23:52 -0700
From:   Bjorn Andersson <andersson@kernel.org>
To:     Konrad Dybcio <konrad.dybcio@linaro.org>
Cc:     Johan Hovold <johan@kernel.org>, Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks
Message-ID: <ybugl2m7o5cnzj4lv5ksit2rip6yvths5ieo3xlw6cycto2zax@2jimga475z2t>
References: <20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org>
 <20230717-topic-branch_aon_cleanup-v1-3-27784d27a4f4@linaro.org>
 <ZLaRtrH85v4kpSvb@hovoldconsulting.com>
 <33a26241-026a-9466-5dd6-e3202b29f57c@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <33a26241-026a-9466-5dd6-e3202b29f57c@linaro.org>
Precedence: bulk

On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote:
> On 18.07.2023 15:20, Johan Hovold wrote:
> > On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
> >> Some clocks need to be always-on, but we don't really do anything
> >> with them, other than calling enable() once and telling Linux they're
> >> enabled.
> >>
> >> Unregister them to save a couple of bytes and, perhaps more
> >> importantly, allow for runtime suspend of the clock controller device,
> >> as CLK_IS_CRITICAL prevents the latter.
> > 
> > But this doesn't sound right. How can you disable a controller which
> > still has clocks enabled?
> > 
> > Shouldn't instead these clocks be modelled properly so that they are
> > only enabled when actually needed?
> Hm.. We do have clk_branch2_aon_ops, but something still needs to
> toggle these clocks.
> 

Before we started replacing these clocks with static votes, I handled
exactly this problem in the turingcc-qcs404 driver by registering the
ahb clock with a pm_clk_add(). The clock framework will then
automagically keep the clock enabled around operations, but it will also
keep the runtime state active as long as the clock is prepared.

As mentioned in an earlier reply today, there's no similarity to this in
the reset or gdsc code, so we'd need to add that in order to rely on
such mechanism.

> I *think* we could leave a permanent vote in probe() without breaking
> runtime pm! I'll give it a spin bit later..
> 

Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would
properly connect the two, and thereby handle probe order between the two
clock controllers.

But it would prevent the power-domain of the parent provider to ever
suspending. Using pm_clk_add() this would at least depend on client
votes.

Regards,
Bjorn