Re: [PATCH 2/2] arm64: dts: apple: t600x: Add PCIe pwren gpios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The device tree changes are non-functional because the Apple PCIe controller driver lacks the required `pci-pwrctrl` framework integration, leaving the endpoints unpowered.
- [High] The fixed regulator nodes for the PCIe ports omit the `startup-delay-us` property, which will cause a violation of the PCIe CEM specification T_PVPERL timing constraint when enabled.
--

--- Patch [2]: [PATCH 2/2] arm64: dts: apple: t600x: Add PCIe pwren gpios ---
commit b2b3991e65bd2ca1aa384e079e00556daa346284
Author: Yureka Lilian <yureka@cyberchaos.dev>

arm64: dts: apple: t600x: Add PCIe pwren gpios
    
This commit adds fixed regulators to enable PCIe peripherals (WiFi, SD card
reader, and USB xHCI) on Apple M1 Pro/Max/Ultra SoCs. It sets these regulators
as the power-supply for the respective PCIe ports and adds the pciclass,0604
compatible string to allow the pci-pwrctrl-generic driver to bind.

> diff --git a/arch/arm64/boot/dts/apple/t600x-die0.dtsi b/arch/arm64/boot/dts/apple/t600x-die0.dtsi
> index f715b19efd167..53a633a3c6527 100644
> --- a/arch/arm64/boot/dts/apple/t600x-die0.dtsi
> +++ b/arch/arm64/boot/dts/apple/t600x-die0.dtsi
[ ... ]
> @@ -449,6 +449,7 @@ pcie0: pcie@590000000 {
>  		pinctrl-names = "default";
>  
>  		port00: pci@0,0 {
> +			compatible = "pciclass,0604";

[Severity: High]
Will this generic power control driver actually be probed?

The pci-pwrctrl-generic driver requires the PCIe host controller to explicitly
call pci_pwrctrl_create_devices() to instantiate platform devices for these
child nodes.

Looking at drivers/pci/controller/pcie-apple.c, this API is never invoked. As
a result, the pci-pwrctrl device is never created, the power-supply properties
will be ignored, and the connected endpoints will remain unpowered.

>  			device_type = "pci";
>  			reg = <0x0 0x0 0x0 0x0 0x0>;
>  			reset-gpios = <&pinctrl_ap 4 GPIO_ACTIVE_LOW>;
> diff --git a/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi b/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
> index fee84f809a9cc..7954cbbe45140 100644
> --- a/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
> +++ b/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
[ ... ]
> @@ -55,6 +55,20 @@ led-0 {
>  			default-state = "keep";
>  		};
>  	};
> +
> +	vreg_pcie0_port0: regulator-pcie0-port0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "pwren-pcie0-port0";
> +		gpio = <&smc_gpio 13 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};

[Severity: High]
Does this regulator need a startup-delay-us property to meet PCIe timing?

When the generic power control framework eventually manages this regulator
(either once the pci-pwrctrl driver is supported or via direct host controller
control), it will enable it without implicit delays. 

Without startup-delay-us = <100000>, regulator_enable() returns immediately.
The apple_pcie_setup_port() function then asserts PERST#, waits only 100us,
and deasserts PERST#. 

Could this result in a T_PVPERL of ~100us instead of the spec-mandated 100ms,
causing the PCIe endpoints to fail initialization or act erratically? (This
also applies to the other fixed regulators introduced in this patch).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-apple-pcie-pwren-v1-0-5a281b182fe2@cyberchaos.dev?part=2