From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Varadarajan Narayanan <quic_varada@quicinc.com>
Cc: Georgi Djakov <djakov@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
andersson@kernel.org, mturquette@baylibre.com, sboyd@kernel.org,
robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
quic_anusha@quicinc.com, linux-arm-msm@vger.kernel.org,
linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v9 6/6] arm64: dts: qcom: ipq9574: Add icc provider ability to gcc
Date: Wed, 8 May 2024 11:10:18 +0300 [thread overview]
Message-ID: <CAA8EJpqENsojPQmCbma_nQLEZq8nK1fz1K0JdtvLd=kPrH_DBw@mail.gmail.com> (raw)
In-Reply-To: <ZjshR0ekcn0gxwOa@hu-varada-blr.qualcomm.com>
On Wed, 8 May 2024 at 09:53, Varadarajan Narayanan
<quic_varada@quicinc.com> wrote:
>
> On Fri, May 03, 2024 at 04:51:04PM +0300, Georgi Djakov wrote:
> > Hi Varada,
> >
> > Thank you for your work on this!
> >
> > On 2.05.24 12:30, Varadarajan Narayanan wrote:
> > > On Tue, Apr 30, 2024 at 12:05:29PM +0200, Konrad Dybcio wrote:
> > > > On 25.04.2024 12:26 PM, Varadarajan Narayanan wrote:
> > > > > On Tue, Apr 23, 2024 at 02:58:41PM +0200, Konrad Dybcio wrote:
> > > > > >
> > > > > >
> > > > > > On 4/18/24 11:23, Varadarajan Narayanan wrote:
> > > > > > > IPQ SoCs dont involve RPM in managing NoC related clocks and
> > > > > > > there is no NoC scaling. Linux itself handles these clocks.
> > > > > > > However, these should not be exposed as just clocks and align
> > > > > > > with other Qualcomm SoCs that handle these clocks from a
> > > > > > > interconnect provider.
> > > > > > >
> > > > > > > Hence include icc provider capability to the gcc node so that
> > > > > > > peripherals can use the interconnect facility to enable these
> > > > > > > clocks.
> > > > > > >
> > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > > > > > > ---
> > > > > >
> > > > > > If this is all you do to enable interconnect (which is not the case,
> > > > > > as this patch only satisfies the bindings checker, the meaningful
> > > > > > change happens in the previous patch) and nothing explodes, this is
> > > > > > an apparent sign of your driver doing nothing.
> > > > >
> > > > > It appears to do nothing because, we are just enabling the clock
> > > > > provider to also act as interconnect provider. Only when the
> > > > > consumers are enabled with interconnect usage, this will create
> > > > > paths and turn on the relevant NOC clocks.
> > > >
> > > > No, with sync_state it actually does "something" (sets the interconnect
> > > > path bandwidths to zero). And *this* patch does nothing functionally,
> > > > it only makes the dt checker happy.
> > >
> > > I understand.
> > >
> > > > > This interconnect will be used by the PCIe and NSS blocks. When
> > > > > those patches were posted earlier, they were put on hold until
> > > > > interconnect driver is available.
> > > > >
> > > > > Once this patch gets in, PCIe for example will make use of icc.
> > > > > Please refer to https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/.
> > > > >
> > > > > The 'pcieX' nodes will include the following entries.
> > > > >
> > > > > interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
> > > > > <&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> > > > > interconnect-names = "pcie-mem", "cpu-pcie";
> > > >
> > > > Okay. What about USB that's already enabled? And BIMC/MEMNOC?
> > >
> > > For USB, the GCC_ANOC_USB_AXI_CLK is enabled as part of the iface
> > > clock. Hence, interconnect is not specified there.
> > >
> > > MEMNOC to System NOC interfaces seem to be enabled automatically.
> > > Software doesn't have to turn on or program specific clocks.
> > >
> > > > > > The expected reaction to "enabling interconnect" without defining the
> > > > > > required paths for your hardware would be a crash-on-sync_state, as all
> > > > > > unused (from Linux's POV) resources ought to be shut down.
> > > > > >
> > > > > > Because you lack sync_state, the interconnects silently retain the state
> > > > > > that they were left in (which is not deterministic), and that's precisely
> > > > > > what we want to avoid.
> > > > >
> > > > > I tried to set 'sync_state' to icc_sync_state to be invoked and
> > > > > didn't see any crash.
> > > >
> > > > Have you confirmed that the registers are actually written to, and with
> > > > correct values?
> > >
> > > I tried the following combinations:-
> > >
> > > 1. Top of tree linux-next + This patch set
> > >
> > > * icc_sync_state called
> > > * No crash or hang observed
> > > * From /sys/kernel/debug/clk/clk_summary can see the
> > > relevant clocks are set to the expected rates (compared
> > > with downstream kernel)
> > >
> > > 2. Top of tree linux-next + This patch set + PCIe enablement
> > >
> > > * icc_sync_state NOT called
> >
> > If sync_state() is not being called, that usually means that there
> > are interconnect consumers that haven't probed successfully (PCIe?)
> > or their dependencies. That can be checked in /sys/class/devlink/.../status
> > But i am not sure how this works for PCI devices however.
> >
> > You can also manually force a call to sync_state by writing "1" to
> > the interconnect provider's /sys/devices/.../state_synced
> >
> > Anyway, the question is if PCIe and NSS work without this driver?
>
> No.
>
> > If they work, is this because the clocks are turned on by default
> > or by the boot loader?
>
> Initially, the PCIe/NSS driver enabled these clocks directly
> by having them in their DT nodes itself. Based on community
> feedback this was removed and after that PCIe/NSS did not work.
>
> > Then if an interconnect path (clock) gets disabled either when we
> > reach a sync_state (with no bandwidth requests) or we explicitly
> > call icc_set_bw() with 0 bandwidth values, i would expect that
> > these PCIe and NSS devices would not function anymore (it might
> > save some power etc) and if this is unexpected we should see a
> > a crash or hang...
> >
> > Can you confirm this?
>
> With ICC enabled, icc_set_bw (with non-zero values) is called by
> PCIe and NSS drivers. Haven't checked with icc_set_bw with zero
> values.
>
> PCIe: qcom_pcie_probe -> qcom_pcie_icc_init -> icc_set_bw
> NSS: ppe_icc_init -> icc_set_bw
>
> I believe sync_state is not getting called since there is a
> non-zero set bandwidth request. Which seems to be aligned with
> your explanation.
This doesn't look correct. sync_state is being called once all
consumers are probed. It doesn't matter whether those consumers have
non-zero bandwidth requests or no.
--
With best wishes
Dmitry
prev parent reply other threads:[~2024-05-08 8:10 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 9:22 [PATCH v9 0/6] Add interconnect driver for IPQ9574 SoC Varadarajan Narayanan
2024-04-18 9:23 ` [PATCH v9 1/6] interconnect: icc-clk: Allow user to specify master/slave ids Varadarajan Narayanan
2024-04-22 23:00 ` Konrad Dybcio
2024-04-25 10:28 ` Varadarajan Narayanan
2024-04-25 16:21 ` Georgi Djakov
2024-04-18 9:23 ` [PATCH v9 2/6] dt-bindings: interconnect: Add Qualcomm IPQ9574 support Varadarajan Narayanan
2024-04-18 17:39 ` Krzysztof Kozlowski
2024-04-18 9:23 ` [PATCH v9 3/6] interconnect: icc-clk: Add devm_icc_clk_register Varadarajan Narayanan
2024-04-18 9:23 ` [PATCH v9 4/6] clk: qcom: common: Add interconnect clocks support Varadarajan Narayanan
2024-04-22 23:05 ` Konrad Dybcio
2024-04-25 10:30 ` Varadarajan Narayanan
2024-04-18 9:23 ` [PATCH v9 5/6] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks Varadarajan Narayanan
2024-04-18 9:23 ` [PATCH v9 6/6] arm64: dts: qcom: ipq9574: Add icc provider ability to gcc Varadarajan Narayanan
2024-04-23 12:58 ` Konrad Dybcio
2024-04-25 10:26 ` Varadarajan Narayanan
2024-04-30 10:05 ` Konrad Dybcio
2024-05-02 9:30 ` Varadarajan Narayanan
2024-05-03 13:51 ` Georgi Djakov
2024-05-08 6:52 ` Varadarajan Narayanan
2024-05-08 8:10 ` Dmitry Baryshkov [this message]
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAA8EJpqENsojPQmCbma_nQLEZq8nK1fz1K0JdtvLd=kPrH_DBw@mail.gmail.com' \
--to=dmitry.baryshkov@linaro.org \
--cc=andersson@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=djakov@kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=quic_anusha@quicinc.com \
--cc=quic_varada@quicinc.com \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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 read-only IMAP folder(s) and NNTP newsgroup(s).