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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The PCIe host controller nodes lack the required 'interrupts' property for native host controller events and legacy INTx IRQ routing properties.
- [High] Incorrect device tree mapping for PCI I/O address ranges.
- [High] Hardware lane configuration mismatch in pcie0_rc specifies 4 lanes but only provisions 2 PHYs.
- [Medium] The 'spacemit,k3-pcie' compatible string is undocumented and its nodes violate established schema constraints.
- [Medium] Inconsistent property placement for 'bus-range' between host controllers.
--

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

riscv: dts: spacemit: k3: Add PCIe device node

This commit adds PCIe device nodes for the Spacemit K3 SoC. It includes
pinctrl configurations and board-level properties for the Pico-ITX board.

> 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]
Does this lane configuration match the provisioned PHYs? This controller
specifies 4 lanes via num-lanes, but only 2 PHYs are provided here. Could
this cause link training failures or PHY framework errors if the driver expects
4 PHYs, especially given lanes 2 and 3 are assigned to pcie2_rc elsewhere?

[Severity: Medium]
Since the phys array contains multiple items, shouldn't there be a
corresponding phy-names array? Omitting it violates standard constraints
for unnamed PHYs in port nodes.

> +	};
> +};
[ ... ]
> diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> index 7863ab7dd308c..314dea88b4a4a 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 this compatible string documented? I couldn't find a device tree binding
schema for spacemit,k3-pcie.

[Severity: High]
Are there missing interrupt properties here? The host controller lacks an
interrupts property for native host controller events like AER, PME, or
link state changes.

Furthermore, is legacy INTx routing supported? Omitting #interrupt-cells,
interrupt-map, and interrupt-map-mask will prevent the PCI subsystem from
routing INTA/B/C/D interrupts for downstream devices.

> +			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";

[Severity: Medium]
Will this pass dt_binding_check validation? The node introduces a fifth
register region named dbi2, which exceeds the typical 4-item limit seen
in similar bindings like spacemit,k1-pcie-host.yaml.

> +			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]
Does this map the I/O window correctly? Mapping the I/O window to start at
a PCI address of 0x00010000 skips the critical first 64KB.

Will this prevent the PCI subsystem from assigning valid I/O BARs for legacy
PCIe cards that expect 16-bit I/O space starting at 0x0000? I notice that
pcie2_rc correctly starts its mapping at 0x00000000.

> +				 <0x02000000 0x0 0x00110000 0x11 0x00110000 0x0 0x7fef0000>,
> +				 <0x43000000 0x18 0x00000000 0x18 0x00000000 0x1 0x00000000>;
[ ... ]
> +		pcie3_rc: pcie@80c00000 {
[ ... ]
> +			linux,pci-domain = <3>;
> +			bus-range = <0x00 0xff>;

[Severity: Medium]
Is the placement of the bus-range property correct here? For the other host
controllers, bus-range is located in the child port node (pcie@0).

Placing it in the parent node instead might cause subsystem enumeration issues
for this bridge.

> +			max-link-speed = <3>;
> +			spacemit,apmu = <&syscon_apmu 0x1e0>;
> +			status = "disabled";
> +
> +			pcie3_port: pcie@0 {
> +				device_type = "pci";
> +				compatible = "pciclass,0604";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
> +		};

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