Re: [PATCH v7 1/3] arm64: dts: qcom: eliza: Add PCIe PHY and controller nodes

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `msi-map` property in PCIe controller nodes.
- [High] Incorrect hardware configuration via copy-paste error: `pcie1` uses SM8550 `pcie0` interrupts and stream IDs.
- [Medium] Missing `num-lanes` property in PCIe controller nodes.
--

--- Patch [1]: [PATCH v7 1/3] arm64: dts: qcom: eliza: Add PCIe PHY and controller nodes ---
commit 2d2260f9f24a370e16b32d6b0fe6e207c07d9ae1
Author: Krishna Chaitanya Chundru <[email protected]>

arm64: dts: qcom: eliza: Add PCIe PHY and controller nodes

Eliza supports two PCIe instances: one 8GT/s x1 (PCIe0) and one 8GT/s x2
(PCIe1). Add PCIe controller and PHY nodes for both instances, and update
the GCC clock references to use the newly added PHY nodes instead of
placeholder zeros.

> diff --git a/arch/arm64/boot/dts/qcom/eliza.dtsi b/arch/arm64/boot/dts/qcom/eliza.dtsi
> index 357e8066c1768..6834137bd5e98 100644
> --- a/arch/arm64/boot/dts/qcom/eliza.dtsi
> +++ b/arch/arm64/boot/dts/qcom/eliza.dtsi
[ ... ]
> +		pcie0: pcie@1c00000 {
> +			device_type = "pci";
> +			compatible = "qcom,eliza-pcie", "qcom,pcie-sm8550";
> +			reg = <0x0 0x01c00000 0x0 0x3000>,
[ ... ]
> +			ranges = <0x01000000 0x0 0x00000000 0x0 0x40200000 0x0 0x100000>,
> +				 <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x3d00000>;
> +

[Severity: High]
Should this node include the msi-map property? Without msi-map mapping PCIe
Requester IDs to the ITS Device IDs, the PCI framework cannot associate the
devices with the GICv3 ITS MSI controller. This could cause endpoints to fail
probing or silently fall back to legacy INTx.

[Severity: Medium]
The commit message mentions PCIe0 is an 8GT/s x1 instance. Does this need a
num-lanes = <1>; property here to restrict the link width? Omitting it
forces the driver to rely on unverified hardware reset defaults.

> +			interrupts = <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
[ ... ]
> +			operating-points-v2 = <&pcie0_opp_table>;
> +
> +			iommu-map = <0x0 &apps_smmu 0x1480 0x0 0x1>,
> +				    <0x100 &apps_smmu 0x1481 0x0 0x1>;

[Severity: High]
Is the stream ID (0x1480) swapped with pcie1? It looks like pcie0 is using
the stream IDs typically reserved for pcie1, while pcie1 below uses 0x1400.
Using incorrect stream IDs could cause immediate SMMU context faults when
endpoints attempt DMA.

[ ... ]
> +		pcie1: pcie@1c08000 {
> +			device_type = "pci";
> +			compatible = "qcom,eliza-pcie", "qcom,pcie-sm8550";
> +			reg = <0x0 0x01c08000 0x0 0x3000>,
[ ... ]
> +			ranges = <0x01000000 0x0 0x00000000 0x0 0x44200000 0x0 0x100000>,
> +				 <0x02000000 0x0 0x44300000 0x0 0x44300000 0x0 0x3d00000>,
> +				 <0x43000000 0x4 0x00000000 0x4 0x00000000 0x3 0x00000000>;
> +

[Severity: High]
Similar to pcie0, should pcie1 include the msi-map property to route its
downstream MSIs to the GIC ITS?

[Severity: Medium]
The commit message mentions PCIe1 is an 8GT/s x2 instance. Should there be a
num-lanes = <2>; property here to prevent relying on unverified hardware
reset defaults for the link width?

> +			interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;

[Severity: High]
Are these interrupts correct for Eliza's pcie1? These look like an exact
copy of the SM8550 pcie0 interrupts (141-148), whereas Eliza's pcie0 uses
distinct interrupts (376, 478...). Wiring pcie1 to incorrect physical IRQ
lines could render endpoint devices dead.

[ ... ]
> +			operating-points-v2 = <&pcie1_opp_table>;
> +
> +			iommu-map = <0x0 &apps_smmu 0x1400 0x0 0x1>,
> +				    <0x100 &apps_smmu 0x1401 0x0 0x1>;

[Severity: High]
Similar to the interrupt assignment, are these stream IDs correct for pcie1?
This uses 0x1400/0x1401 which are typically SM8550 pcie0 IDs, whereas pcie0
above was assigned 0x1480.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1