From: Stephen Boyd <email@example.com>
To: Bjorn Andersson <firstname.lastname@example.org>
Cc: Mark Rutland <email@example.com>,
Michael Turquette <firstname.lastname@example.org>,
Rob Herring <email@example.com>, Andy Gross <firstname.lastname@example.org>,
Paolo Pisati <email@example.com>
Subject: Re: [PATCH 1/2] clk: qcom: gcc-msm8996: Fix parent for CLKREF clocks
Date: Thu, 26 Dec 2019 17:35:27 -0800 [thread overview]
Message-ID: <20191227013528.A445B2080D@mail.kernel.org> (raw)
Quoting Bjorn Andersson (2019-12-26 14:41:56)
> On Mon 23 Dec 18:20 PST 2019, Stephen Boyd wrote:
> > Quoting Bjorn Andersson (2019-12-19 18:34:27)
> > > On Wed 18 Dec 22:37 PST 2019, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2019-12-07 12:36:02)
> > > > > The CLKREF clocks are all fed by the clock signal on the CXO2 pad on the
> > > > > SoC. Update the definition of these clocks to allow this to be wired up
> > > > > to the appropriate clock source.
> > > > >
> > > > > Retain "xo" as the global named parent to make the change a nop in the
> > > > > event that DT doesn't carry the necessary clocks definition.
> > > >
> > > > Something seems wrong still.
> > > >
> > > > I wonder if we need to add the XO "active only" clk to the rpm clk
> > > > driver(s) and mark it as CLK_IS_CRITICAL. In theory that is really the
> > > > truth for most of the SoCs out there because it's the only crystal that
> > > > needs to be on all the time when the CPU is active. The "normal" XO clk
> > > > will then be on all the time unless deep idle is entered and nobody has
> > > > turned that on via some clk_prepare() call. That's because we root all
> > > > other clks through the "normal" XO clk that will be on in deep
> > > > idle/suspend if someone needs it to be.
> > > >
> > >
> > > The patch doesn't attempt to address the fact that our representation of
> > > XO is incomplete, only the fact that CXO2 isn't properly described.
> > >
> > > Looking at the clock distribution, we do have RPM_SMD_BB_CLK1_A which
> > > presumably is the clock you're referring to here - i.e. the clock
> > > resource connected to CXO.
> > I don't mean the buffer clks, but the XO resource specifically. It's the
> > representation to the RPM that deep sleep/deep idle should or shouldn't
> > turn off XO and achieve "XO shutdown". Basically it can never be off
> > when the CPU is active because then the CPU itself wouldn't be clocked,
> > but when the CPU isn't active we may want to turn it off if nothing is
> > using it during sleep to clock some sort of wakeup logic or device that
> > is active when the CPU is idle.
> I see. So we're missing the representation of the "raw" CXO in
> clk-smd-rpm.c, and I'm lacking some understanding of how these pieces
> should be tied together for us to realize the "XO shutdown"...
Ok. This is another topic so not important to this patch right now.
> > >
> > > > Did the downstream code explicitly enable this ln_bb_clk in the phy
> > > > drivers? I think it may have?
> > > >
> > >
> > > Yes, afaict all downstream drivers consuming a CLKREF also consumes
> > > LN_BB and ensures that this is enabled. So we've been relying on UFS to
> > > either not have probed yet or that UFS probed successfully for PCIe and
> > > USB to be functional.
> > >
> > > So either we need this patch to ensure that the requests propagates
> > > down, or I need to patch up the PHY drivers to ensure that they also
> > > vote for the PMIC clock - and I do prefer this patch.
> > Cool. Yeah seems better to just indicate that the reference clks are
> > clocked by something else and fix that problem now.
> Let me know if I shouldn't interpret this sentence as "let's merge this
> for now".
Yes I'd like to merge for now but the binding needs to be adjusted.
next prev parent reply other threads:[~2019-12-27 1:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-07 20:36 [PATCH 0/2] clk: qcom: gcc-msm8996: Fix CLKREF parenting Bjorn Andersson
2019-12-07 20:36 ` [PATCH 1/2] clk: qcom: gcc-msm8996: Fix parent for CLKREF clocks Bjorn Andersson
2019-12-18 23:43 ` Rob Herring
2019-12-19 6:37 ` Stephen Boyd
2019-12-20 2:34 ` Bjorn Andersson
2019-12-24 2:20 ` Stephen Boyd
2019-12-26 22:41 ` Bjorn Andersson
2019-12-27 1:35 ` Stephen Boyd [this message]
2019-12-24 2:48 ` Stephen Boyd
2019-12-26 22:23 ` Bjorn Andersson
2019-12-26 22:34 ` Stephen Boyd
2019-12-07 20:36 ` [PATCH 2/2] arm64: dts: qcom: msm8996: Define parent clocks for gcc Bjorn Andersson
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).