Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Shawn Guo <shawn.guo@linaro.org>
Cc: Stephan Gerhold <stephan@gerhold.net>,
	Stephen Boyd <sboyd@kernel.org>,
	Loic Poulain <loic.poulain@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/3] clk: qcom: smd-rpm: Report enable state to framework
Date: Mon, 6 Dec 2021 09:42:52 -0600	[thread overview]
Message-ID: <Ya4vfCZ8UTVBRpYU@builder.lan> (raw)
In-Reply-To: <20211111093926.GL7231@dragon>

On Thu 11 Nov 03:39 CST 2021, Shawn Guo wrote:

> On Wed, Nov 10, 2021 at 05:48:10AM -0800, Bjorn Andersson wrote:
> > > IMHO, properly reporting enable state to framework is definitely the
> > > right thing to do, and should have been done from day one.
> > > 
> > 
> > I always thought is_enabled() should reflect the hardware state - in
> > particular for clk_summary. The particular concern being that by
> > initializing the is_enabled() state to either true or false, we're
> > making an assumption about the hardware state. And if something where to
> > do if (enabled) disable (or if (disabled) enable), we might skip a
> > critical operation just because we tricked the logic.
> 
> That's probably why clk_smd_rpm_handoff() is called.  As there is no way
> to query RPM for resource state, we send enable request for all RPM
> clocks to get hardware and software state in sync.
> 

clk_smd_rpm_handoff() will ensure that all SMD clocks are enabled, and
at max speed during rpm_smd_clk_probe(). Once clients starts actually
voting for rates that will change.

(Un)fortunately as we don't provide an implementation of is_enabled()
clk_disable_unused() won't try to turn them off. This similar to a
problem I have elsewhere, for which I proposed:
https://lore.kernel.org/linux-arm-msm/20211203035436.3505743-1-bjorn.andersson@linaro.org/

We should at some point introduce this for the SMD clocks as well.

However, we have two problems:
1) Compiling e.g. the interconnect provider as a module would mean that
clk_disable_unused() kicks in before the client has had a chance to vote
for the clock.

2) One client may enable the clock during its probe and then disable it.
Being the last active user the clock framework happily turns off the
clock.

For both of these cases, we need to ensure that the clocks aren't
disabled until sync_state() kicks in.

Regards,
Bjorn

> > So, do you need it for anything other than clk_disable_unused()?
> 
> Not critical, but I need it for debugfs clk_summary as well.
> 
> Shawn