MIME-Version: 1.0
References: <20200817140908.185976-1-stephan@gerhold.net> <CAOCk7Nq6CT5q_aXG2jZ2t5=3YKVKM4r=gSnJLJkVccpwyc3XnQ@mail.gmail.com>
 <20200817152848.GA836@gerhold.net>
In-Reply-To: <20200817152848.GA836@gerhold.net>
From: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Date: Mon, 17 Aug 2020 09:46:08 -0600
Message-ID: <CAOCk7NpyiWO_DHidDWbwdBYbzJMrv26CmWOR4foTGRL_pQVbUQ@mail.gmail.com>
Subject: Re: [PATCH] clk: qcom: smd: Disable unused clocks
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>, 
	Bjorn Andersson <bjorn.andersson@linaro.org>, Michael Turquette <mturquette@baylibre.com>, 
	MSM <linux-arm-msm@vger.kernel.org>, linux-clk@vger.kernel.org, 
	~postmarketos/upstreaming@lists.sr.ht, 
	Georgi Djakov <georgi.djakov@linaro.org>
Content-Type: text/plain; charset="UTF-8"

On Mon, Aug 17, 2020 at 9:29 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Mon, Aug 17, 2020 at 08:52:46AM -0600, Jeffrey Hugo wrote:
> > > Overall I'm not entirely sure why we need to force all these clocks
> > > on at all... But the downstream driver also seems to do it and the RPM
> > > interface is barely documented, so I didn't feel comfortable changing it...
> >
> > So essentially, when the clk framework goes through late init, and
> > decides to turn off clocks that are not being used, it will also turn
> > off these clocks?
> >
>
> With this patch: yes.
>
> > I think this is going to break other targets where other subsystems
> > happen to rely on these sorts of votes from Linux inorder to run/boot
> > (not saying it's a good thing, just that is how it is and since we
> > can't change the FW on those....).
> >
>
> As far as I can tell the behavior implemented in this patch (= force
> clocks on during boot but disable them when unused) is the same on that
> is used on the downstream kernel. Most FW is probably written with the
> downstream kernel in mind, so I don't think this is going to cause trouble.

Based on my experience with 8998, I disagree.  I would need to dig up
the history for specifics.

>
> The only situation this patch could break something is if we forgot to
> manage the clocks for one of the devices in mainline
> (and implicitly relied on clk-smd-rpm to keep them always-on).
>
> For example, one situation I checked is for WCNSS on MSM8916.
> It seems to require RF_CLK2 to boot. However, this is already handled in
> qcom_wcnss_iris.c where the clock is forced on until WCNSS is ready.
>
> > I think this needs to be validated on every single qcom platform using
> > this driver.
> >
> > Also, out of curiosity, how are you validating that BB_CLK2 is
> > actually off after this change?
> >
>
> Since BB_CLK1/2 and RF_CLK1/2 are part of the PMIC (at least on MSM8916)
> I used the regmap debugfs interface to read the clock registers
> through SPMI from Linux.
>
> From the "PM8916 Hardware Register Description" [1] I got the registers
> mentioned in the table, e.g. for BB_CLK2:
>
> 0x5208: BB_CLK2_STATUS1
>         BIT(7): CLK_OK (Indicates Hardware or Software enable and
>                         includes warmup delay)
>                 0x0: BBCLK_OFF
>                 0x1: BBCLK_ON
>
> I read the registers from /sys/kernel/debug/regmap/0-00/registers:
>
> Without this patch:
>         5108: 80
>         5208: 80
>         5408: 80
>         5508: 80
>
> With this patch (and with clk-smd-rpm entirely disabled):
>         5108: 80
>         5208: 00
>         5408: 00
>         5508: 00
>
> Stephan
>
> [1]: https://developer.qualcomm.com/download/sd410/pm8916-hardware-register-description.pdf

Hmm, 8916 is probably old enough where you can actually do that.  For
the modern SoCs, you'll have to go through jtag to get an accurate
view of the clocks.