Re: [PATCH 8/8] arm64: dts: qcom: msm8998: wire up interconnect paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: qcom: msm8998: sdhc2 interconnect unvoted without OPP table
- [Medium] arm64: dts: qcom: msm8998: gpu gfx-mem path unvoted without opp-peak-kBps
--

commit dd572379ba20bf853f70a81ced03581edf3082ef
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

arm64: dts: qcom: msm8998: wire up interconnect paths

This commit adds interconnect paths to the GPU, MDSS, Venus, UFS, USB, and
SDHC2 nodes in msm8998.dtsi so that consumer drivers can vote for NoC and DDR
bandwidth.

> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index bb004e0b74ef2..939601322cfb2 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi

[ ... ]

> @@ -1622,6 +1629,10 @@ adreno_gpu: gpu@5000000 {
>  				"rbcpr",
>  				"core";
>  
> +			interconnects = <&bimc MASTER_OXILI QCOM_ICC_TAG_ALWAYS
> +					 &bimc SLAVE_EBI QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "gfx-mem";
> +
>  			interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
>  			iommus = <&adreno_smmu 0>;
>  			operating-points-v2 = <&gpu_opp_table>;

[Severity: Medium]
Will the gfx-mem interconnect path actually be acquired and voted on without
opp-peak-kBps entries in gpu_opp_table?

For Adreno A5xx (qcom,adreno-540.1), the driver does not call devm_of_icc_get()
directly. Instead, adreno_bind() in drivers/gpu/drm/msm/adreno/adreno_device.c
uses the OPP core:

	ret = dev_pm_opp_of_find_icc_paths(dev, NULL);
	if (ret)
		return ret;

In drivers/opp/of.c, dev_pm_opp_of_find_icc_paths() calls
_bandwidth_supported(), which checks the first child node of gpu_opp_table for
opp-peak-kBps:

	prop = of_find_property(np, "opp-peak-kBps", NULL);
	if (!prop || !prop->length)
		return 0;

Because the OPP nodes in gpu_opp_table in arch/arm64/boot/dts/qcom/msm8998.dtsi
do not define opp-peak-kBps, _bandwidth_supported() returns 0 and
dev_pm_opp_of_find_icc_paths() skips parsing the gfx-mem path. Should
gpu_opp_table also be populated with opp-peak-kBps values?

[ ... ]

> @@ -2355,6 +2372,12 @@ sdhc2: mmc@c0a4900 {
>  				     <GIC_SPI 221 IRQ_TYPE_LEVEL_HIGH>;
>  			interrupt-names = "hc_irq", "pwr_irq";
>  
> +			interconnects = <&a2noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
> +					 &bimc SLAVE_EBI QCOM_ICC_TAG_ALWAYS>,
> +					<&gnoc MASTER_APSS_PROC QCOM_ICC_TAG_ALWAYS
> +					 &cnoc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "sdhc-ddr", "cpu-sdhc";
> +
>  			clock-names = "iface", "core", "xo";
>  			clocks = <&gcc GCC_SDCC2_AHB_CLK>,
>  				 <&gcc GCC_SDCC2_APPS_CLK>,

[Severity: High]
Can adding interconnects to sdhc2 without an operating-points-v2 table cause
the A2NoC bus clock and bandwidth votes to drop to zero after icc_sync_state()?

In drivers/mmc/host/sdhci-msm.c, sdhci_msm_probe() relies on the OPP core to
acquire interconnect paths rather than calling of_icc_get() directly:

	/* Check for optional interconnect paths */
	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
	if (ret)
		goto bus_clk_disable;

	ret = devm_pm_opp_set_clkname(&pdev->dev, "core");
	if (ret)
		goto bus_clk_disable;

	/* OPP table is optional */
	ret = devm_pm_opp_of_add_table(&pdev->dev);

In drivers/opp/of.c, dev_pm_opp_of_find_icc_paths() calls
_bandwidth_supported(), which returns 0 when operating-points-v2 is absent or
lacks opp-peak-kBps:

	/* Lets not fail in case we are parsing opp-v1 bindings */
	if (!opp_np)
		return 0;

As a result, sdhc-ddr and cpu-sdhc are never acquired or voted on. Because
sdhc2 is the only consumer referencing &a2noc in msm8998.dtsi (and
msm8998_a2noc sets keep_alive = false), once sdhc2 finishes probing,
icc_sync_state() runs on a2noc, clears the initial INT_MAX boot floor on
mas_sdcc_2 and slv_a2noc_snoc, and drops the A2NoC RPM bandwidth votes and
aggre2_clk to 0 even while sdhc2 is active.

Should sdhc2 include an operating-points-v2 table with opp-peak-kBps entries?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-msm8998-icc-v1-0-c2da3934b6c8@oss.qualcomm.com?part=8