Re: [PATCH 2/2] riscv: dts: spacemit: k3: Add PCIe device node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect and inconsistent PCI I/O space mapping in the ranges property.
- [High] PCIe root complex nodes are missing mandatory interrupt properties.
- [High] pcie0_rc requests 4 lanes but only provides 2 PHYs.
- [High] The phys and vpcie3v3-supply properties are incorrectly nested inside the child nodes.
- [Medium] Undocumented compatible string 'spacemit,k3-pcie' prevents schema validation.
- [Medium] The bus-range property is incorrectly placed on the child PCI-PCI bridge node.
--

--- Patch [2]: [PATCH 2/2] riscv: dts: spacemit: k3: Add PCIe device node ---
commit d6aabd33ce9c5592fb5dc09875e34a446d2e973d
Author: Inochi Amaoto <inochiama@gmail.com>

riscv: dts: spacemit: k3: Add PCIe device node
    
Add all PCIe device node for Spacemit K3.

> diff --git a/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts b/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts
> index 2a6d35a64d5ce..1eb5abbc61f97 100644
> --- a/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts
> +++ b/arch/riscv/boot/dts/spacemit/k3-pico-itx.dts
> @@ -205,6 +205,44 @@ phy0: phy@1 {
>  	};
>  };
>  
> +&pcie0_rc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie0_0_cfg>;
> +	num-lanes = <4>;
> +	status = "okay";
> +
> +	pcie@0 {
> +		vpcie3v3-supply = <&reg_aux_vcc3v3>;
> +		phys = <&combophy 0 PHY_TYPE_PCIE>,
> +		       <&combophy 1 PHY_TYPE_PCIE>;
> +	};

[Severity: High]
Are there PHY definitions missing here? The pcie0_rc node requests 4 lanes 
(num-lanes = <4>) but only provides 2 PHYs in the phys array. This mismatch 
typically causes controller initialization failures or degraded links.

[Severity: High]
Should the vpcie3v3-supply and phys properties be placed in the parent 
pcie0_rc node instead of the pcie@0 child node? Standard host controller 
drivers typically query regulators and phys on the host controller's own 
device node. If they are nested under the bridge node, the regulator query 
will fail and the slot may not receive power.

[ ... ]

> diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> index 82c9e2da82e9f..53cfb2ad48cf7 100644
> --- a/arch/riscv/boot/dts/spacemit/k3.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k3.dtsi
> @@ -439,6 +439,201 @@ soc: soc {
>  		dma-noncoherent;
>  		ranges;
>  
> +		pcie0_rc: pcie@80000000 {
> +			compatible = "spacemit,k3-pcie";

[Severity: Medium]
Is there a companion patch that adds the DT binding documentation for 
spacemit,k3-pcie? Without the schema documentation, dt_binding_check 
cannot validate constraints and required properties.

[Severity: High]
Does this PCIe root complex node need interrupts, #interrupt-cells, and 
interrupt-map properties? Without these mandatory interrupt properties, 
legacy INTx routing, AER, and power management host events will typically 
fail to function.

> +			reg = <0x0 0x80000000 0x0 0x00001000>,
> +			      <0x0 0x80300000 0x0 0x00003f20>,
> +			      <0x11 0x00000000 0x0 0x00010000>,
> +			      <0x0 0x82900000 0x0 0x00001000>,
> +			      <0x0 0x80100000 0x0 0x00001000>;
> +			reg-names = "dbi", "atu", "config", "link", "dbi2";
> +			device_type = "pci";
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			clocks = <&syscon_apmu CLK_APMU_PCIE_PORTA_DBI>,
> +				 <&syscon_apmu CLK_APMU_PCIE_PORTA_MSTE>,
> +				 <&syscon_apmu CLK_APMU_PCIE_PORTA_SLV>;
> +			clock-names = "dbi", "mstr", "slv";
> +			msi-parent = <&simsic>;
> +			ranges = <0x01000000 0x00 0x00010000 0x11 0x00010000 0x0 0x00100000>,

[Severity: High]
Is the PCI I/O space child address intentionally mapped to 0x00010000 
instead of 0x00000000? Legacy PCI endpoints expecting standard I/O BAR 
behavior might fail since the base address is shifted beyond the typical 
16-bit legacy I/O boundary.

> +				 <0x02000000 0x0 0x00110000 0x11 0x00110000 0x0 0x7fef0000>,
> +				 <0x43000000 0x18 0x00000000 0x18 0x00000000 0x1 0x00000000>;
> +			resets = <&syscon_apmu RESET_APMU_PCIE_A_DBI>,
> +				 <&syscon_apmu RESET_APMU_PCIE_A_MASTER>,
> +				 <&syscon_apmu RESET_APMU_PCIE_A_SLAVE>;
> +			reset-names = "dbi", "mstr", "slv";
> +			max-link-speed = <3>;
> +			linux,pci-domain = <0>;
> +			spacemit,apmu = <&syscon_apmu 0x1f0>;
> +			status = "disabled";
> +
> +			pcie0_port: pcie@0 {
> +				device_type = "pci";
> +				compatible = "pciclass,0604";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;

[Severity: Medium]
Should the bus-range property be located in the parent pcie0_rc node 
rather than here in the child PCI-PCI bridge node? The PCI core subsystem 
relies on bus-range at the host controller level to properly define the 
decoded buses.

> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
> +		};
> +
> +		pcie1_rc: pcie@80400000 {
> +			compatible = "spacemit,k3-pcie";

[ ... ]

> +			ranges = <0x01000000 0x0 0x00010000 0x11 0x80010000 0x0 0x00100000>,

[Severity: High]
Does this mapping also have the same offset issue for the child PCI address, 
starting at 0x00010000 instead of 0x00000000?

[ ... ]

> +		pcie3_rc: pcie@80c00000 {
> +			compatible = "spacemit,k3-pcie";

[ ... ]

> +			ranges = <0x01000000 0x0 0x00010000 0x12 0x80010000 0x0 0x100000>,

[Severity: High]
Similarly, is this child PCI address mapping offset intentionally starting 
at 0x00010000 here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709040415.977784-1-inochiama@gmail.com?part=2