Received-SPF: pass (google.com: domain of linux-kernel+bounces-166226-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 2 May 2024 15:00:54 +0530
From: Varadarajan Narayanan <quic_varada@quicinc.com>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
CC: <andersson@kernel.org>, <mturquette@baylibre.com>, <sboyd@kernel.org>,
        <robh@kernel.org>, <krzk+dt@kernel.org>, <conor+dt@kernel.org>,
        <djakov@kernel.org>, <dmitry.baryshkov@linaro.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
Message-ID: <ZjNdTmmXucjtRxJt@hu-varada-blr.qualcomm.com>
References: <20240418092305.2337429-1-quic_varada@quicinc.com>
 <20240418092305.2337429-7-quic_varada@quicinc.com>
 <a7194edd-a2c8-46fc-bea1-f26b0960e535@linaro.org>
 <Ziov6bWBXYXJ4Zp8@hu-varada-blr.qualcomm.com>
 <27f4f3dd-9375-40cf-8c8f-1c4edf66e31b@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <27f4f3dd-9375-40cf-8c8f-1c4edf66e31b@linaro.org>

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
	* 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)

Does this answer your question? Please let me know if you were
looking for some other information.

Thanks
Varada