Re: [PATCH 2/2] interconnect: qcom: Add MSM8909 interconnect provider driver

From: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Georgi Djakov <djakov@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Adam Skladowski <a39.skl@gmail.com>
Subject: Re: [PATCH 2/2] interconnect: qcom: Add MSM8909 interconnect provider driver
Date: Wed, 20 Dec 2023 11:21:30 +0100	[thread overview]
Message-ID: <ZYLAKvXE156fPHJ1@kernkonzept.com> (raw)
In-Reply-To: <3f921a53-8eac-48e6-be17-43c07a0313d0@linaro.org>

On Sat, Dec 09, 2023 at 06:54:16PM +0100, Konrad Dybcio wrote:
> On 6.12.2023 15:35, Stephan Gerhold wrote:
> > From: Adam Skladowski <a39.skl@gmail.com>
> > 
> > Add driver for interconnect busses found in MSM8909 based platforms.
> > The topology consists of three NoCs that are partially controlled by a
> > RPM processor.
> > 
> > In the downstream/vendor kernel from Qualcomm there is an additional
> > "mm-snoc". However, it doesn't have a separate RPM clock assigned. It
> > looks like this is actually the same NoC in hardware and the "mm-snoc"
> > was only defined to assign a different "qcom,util-fact". In mainline we
> > can represent this by assigning the equivalent "ab_coeff" to all the
> > nodes that are part of "mm-snoc" downstream.
> Worth mentioning that snoc_mm used the same clock as snoc, which is not
> obvious with some of these older designs:
> 
> static DEFINE_CLK_VOTER(snoc_msmbus_a_clk,  &snoc_a_clk.c,  LONG_MAX);
> static DEFINE_CLK_VOTER(snoc_mm_msmbus_a_clk,  &snoc_a_clk.c,  LONG_MAX);
> 

Thanks, will try to clarify this in v2!

> [...]
> 
> > +
> > +static struct platform_driver msm8909_noc_driver = {
> > +	.probe = qnoc_probe,
> > +	.remove = qnoc_remove,
> > +	.driver = {
> > +		.name = "qnoc-msm8909",
> > +		.of_match_table = msm8909_noc_of_match,
> > +		.sync_state = icc_sync_state,
> > +	},
> > +};
> > +module_platform_driver(msm8909_noc_driver);
> And you may want this to be a bit higher up in the food chain!

I would prefer to keep this as-is for now since this is what it has been
intensively tested with. I do have a timing issue when interconnect is
configured for the debug UART, but moving to core_initcall() does not
help either. It seems like the ICC driver simply cannot be probed
earlier due to other dependencies, or due to the time the RPM needs
until it shows up.

At some point we should do a thorough review of the initcall levels for
all the Qualcomm-related core drivers and see if and in which cases
diverting from the defaults actually helps. I would leave this up for
future work right now, though.

> 
> Otherwise I don't see anything obviously wrong, I assume you tested
> this without clk/pd_ignore_unused, with rpmcc cleanup and can confirm
> the QoS programming went through without angry resets.
> 

Yes, all of that works in my tests. 🎉

Thanks for the review!

-- 
Stephan Gerhold <stephan.gerhold@kernkonzept.com>
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth