Re: [PATCH v5 3/4] arm64: dts: qcom: shikra: add PCIe PHY and controller nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] arm64: dts: qcom: shikra: missing 4th GIC cell in pcie
  interrupt-map
- [High] arm64: dts: qcom: shikra: wrong slave and ICC tags in pcie
  interconnects
- [High] arm64: dts: qcom: shikra: missing pcie_phy clock phandle in gcc
  node
- [Medium] arm64: dts: qcom: shikra: 1-cell iommu-map entries for 2-cell
  apps_smmu
--

commit 5f4c78a483eb233ec45c85fdfcd5ba4cf541044d
Author: Sushrut Shree Trivedi <sushrut.trivedi@oss.qualcomm.com>

arm64: dts: qcom: shikra: add PCIe PHY and controller nodes

This commit adds the PCIe controller and QMP Gen2x1 PHY device tree nodes
for the Qualcomm Shikra SoC.

> diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
> index 0a95ad5a5582c..fa9b0d41a576c 100644
> --- a/arch/arm64/boot/dts/qcom/shikra.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra.dtsi
> @@ -1285,6 +1285,164 @@ rng: rng@4454000 {
>  			reg = <0x0 0x04454000 0x0 0x1000>;
>  		};
>  
> +		pcie: pcie@45e8000 {

[ ... ]

> +			interrupt-map = <0 0 0 1 &intc 0 0 0 499 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 2 &intc 0 0 0 500 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 3 &intc 0 0 0 501 IRQ_TYPE_LEVEL_HIGH>,
> +					<0 0 0 4 &intc 0 0 0 502 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			#interrupt-cells = <1>;

[Severity: High]
Are these interrupt-map entries missing the fourth GICv3 interrupt cell?

In shikra.dtsi, &intc declares #address-cells = <2> and
#interrupt-cells = <4> (as used in the interrupts property above).  Each
entry here only provides 5 cells after &intc instead of 6, so
of_irq_parse_imap_parent() consumes the first cell of the next entry and
shifts INTB, INTC, and INTD out of alignment, causing legacy INTx mapping
for those pins to fail with -EINVAL.

[ ... ]

> +			interconnects = <&system_noc MASTER_PCIE2_0 QCOM_ICC_TAG_ALWAYS
> +					&mc_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
> +					<&mem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
> +					&config_noc SLAVE_PCIE2_0 QCOM_ICC_TAG_ACTIVE_ONLY>;
> +
> +			interconnect-names = "pcie-mem",
> +					     "cpu-pcie";

[Severity: High]
Does the cpu-pcie interconnect path point to the wrong slave node and use
RPMh tags on an SMD-RPM platform?

SLAVE_PCIE2_0 (29) is a slave on &system_noc (shikra_sys_noc_nodes),
whereas index 29 in shikra_config_noc_nodes (&config_noc) is
SLAVE_PKA_WRAPPER_CFG, so cpu-pcie votes on qhs_pka_wrapper instead of
PCIe (&system_noc SLAVE_PCIE2_0 or &config_noc SLAVE_PCIE_CFG).  In
addition, Shikra uses the SMD-RPM interconnect driver (icc-rpm.c), where
RPM_ALWAYS_TAG and RPM_ACTIVE_TAG should be used instead of RPMh's
QCOM_ICC_TAG_ALWAYS and QCOM_ICC_TAG_ACTIVE_ONLY (0x3), which sets BIT(1)
and keeps active-only bandwidth voted during system sleep.

> +
> +			iommu-map = <0x0 &apps_smmu 0x800 0x1>,
> +				    <0x100 &apps_smmu 0x801 0x1>;

[Severity: Medium]
Should each iommu-map entry include a second IOMMU specifier cell (such as
<0x0 &apps_smmu 0x800 0x0 0x1>)?

&apps_smmu declares #iommu-cells = <2>, so 4-cell iommu-map entries trigger
runtime warnings in qcom_pcie_config_sid_1_9_0() and of_map_id() as well as
dtbs_check schema errors.

[ ... ]

> +		pcie_phy: phy@45ee000 {

[ ... ]

> +			#clock-cells = <0>;
> +			clock-output-names = "pcie_pipe_clk";
> +
> +			#phy-cells = <0>;
> +
> +			status = "disabled";
> +		};

[Severity: High]
Should the gcc node's clocks property also be updated to reference
<&pcie_phy> at index 6 (DT_PCIE_PIPE_CLK)?

In drivers/clk/qcom/gcc-shikra.c, gcc_pcie_pipe_clk_src resolves its parent
solely through DT index DT_PCIE_PIPE_CLK (6) on &gcc:

drivers/clk/qcom/gcc-shikra.c:
static struct clk_regmap_phy_mux gcc_pcie_pipe_clk_src = {
	.reg = 0xaf058,
	.clkr = {
		.hw.init = &(const struct clk_init_data) {
			.name = "gcc_pcie_pipe_clk_src",
			.parent_data = &(const struct clk_parent_data) {
				.index = DT_PCIE_PIPE_CLK,
			},
			.num_parents = 1,
			.ops = &clk_regmap_phy_mux_ops,
		},
	},
};

Because index 6 of &gcc's clocks property in shikra.dtsi is still <0>,
gcc_pcie_pipe_clk_src and gcc_pcie_pipe_clk remain orphaned and never
parent to pcie_phy's clock provider.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112083355.3884737-1-linghui.wu@oss.qualcomm.com?part=3