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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] arm64: dts: apple: t600x PCIe regulators break PCIe power on t602x boards
- [Medium] PCI: apple: missing pwrctrl power-off and cleanup on error in apple_pcie_init()
- [Low] arm64: dts: apple: undocumented power-supply property on PCIe bridge nodes
- [Low] PCI/pwrctrl: missing !CONFIG_PCI_PWRCTRL stubs for per-device pwrctrl APIs
- [Low] PCI/pwrctrl: mismatched kernel-doc parameter names in drivers/pci/pwrctrl/core.c
--

commit fc220e60fe2546bbee22affe9598f38be84ac7dc
Author: Yureka Lilian <yureka@cyberchaos.dev>

arm64: dts: apple: t600x: Add PCIe pwren gpios

This series exports per-device PCI power control helpers, integrates them into
the Apple PCIe controller driver, and adds SMC GPIO-controlled fixed
regulators for PCIe root ports on Apple T600x devicetrees.

> diff --git a/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi b/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
> index caded1636f3d2..4f9dccf839ad0 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;
> +	};
> +
> +	vreg_pcie0_port1: regulator-pcie0-port1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "pwren-pcie0-port1";
> +		gpio = <&smc_gpio 26 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
>  };

[Severity: High]
Will adding these regulators to t600x-j314-j316.dtsi and t600x-j375.dtsi cut
power to PCIe endpoints on T602x systems?

Both files are included by the T602x board files:

arch/arm64/boot/dts/apple/t602x-j414-j416.dtsi:
    /*
     * These models are essentially identical to the previous generation, other than
     * the GPIO indices.
     */

    #include "t600x-j314-j316.dtsi"

arch/arm64/boot/dts/apple/t602x-j474-j475.dtsi:
    /*
     * These models are very similar to the previous generation Mac Studio, other
     * than GPIO indices.
     */

    #include "t600x-j375.dtsi"

Because vreg_pcie0_port0, vreg_pcie0_port1, and vreg_pcie0_port3 do not set
regulator-boot-on, reg_fixed_voltage_probe() requests each SMC GPIO with
GPIOD_OUT_LOW and immediately drives the pin low via macsmc_gpio_set().

However, port00 through port03 in arch/arm64/boot/dts/apple/t602x-die0.dtsi do
not have compatible = "pciclass,0604":

arch/arm64/boot/dts/apple/t602x-die0.dtsi:
		port00: pci@0,0 {
			device_type = "pci";
			reg = <0x0 0x0 0x0 0x0 0x0>;
			reset-gpios = <&pinctrl_ap 4 GPIO_ACTIVE_LOW>;

As a result, pci_pwrctrl_is_required() returns false on T602x, no pwrctrl
device binds to re-enable the regulators after reg_fixed_voltage_probe() turns
them off, the T602x files note that GPIO indices differ from T600x, and
arch/arm64/boot/dts/apple/t6020-j474s.dts has an unused port01 where sdhci0 is
deleted (/delete-node/ &sdhci0;) while vreg_pcie0_port1 still claims smc_gpio
26.

[ ... ]

> @@ -176,6 +190,7 @@ &nco_clkref {
>  /* PCIe devices */
>  &port00 {
>  	/* WLAN */
> +	power-supply = <&vreg_pcie0_port0>;
>  	bus-range = <1 1>;
>  	wifi0: wifi@0,0 {
>  		compatible = "pci14e4,4433";
> @@ -195,6 +210,7 @@ bluetooth0: bluetooth@0,1 {
>  
>  &port01 {
>  	/* SD card reader */
> +	power-supply = <&vreg_pcie0_port1>;
>  	bus-range = <2 2>;
>  	sdhci0: mmc@0,0 {
>  		compatible = "pci17a0,9755";

[Severity: Low]
Should port00 and port01 here (and port00, port01, and port03 in
arch/arm64/boot/dts/apple/t600x-j375.dtsi) use a documented PCI slot supply
property such as vpcie3v3-supply instead of power-supply?

Documentation/devicetree/bindings/pci/apple,pcie.yaml sets
unevaluatedProperties: false and references /schemas/pci/pci-host-bridge.yaml#
for child bridge nodes, where power-supply is not a documented property and
causes dtbs_check warnings, whereas vpcie3v3-supply is defined by the PCI slot
schema and consumed by pci-pwrctrl-slot via of_pci_supply_present() and
of_regulator_bulk_get_all().

[ ... ]

> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index c2cffc0659f44..2d32092e2122b 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -30,6 +30,7 @@
>  #include <linux/msi.h>
>  #include <linux/of_irq.h>
>  #include <linux/pci-ecam.h>
> +#include <linux/pci-pwrctrl.h>
>  
>  #include "pci-host-common.h"
>  
> @@ -825,6 +826,28 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  	if (WARN_ON(!pcie))
>  		return -ENOENT;
>  
> +	// Ensure all ports can be powered on before attempting initialization
> +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		ret = pci_pwrctrl_create_device(of_port, pcie->dev);
> +		if (ret) {
> +			dev_err(pcie->dev, "Failed to create pwrctrl device: %pe\n", ERR_PTR(ret));
> +			return ret;
> +		}
> +
> +		ret = pci_pwrctrl_power_on_device(of_port);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER) {
> +				dev_err(pcie->dev,
> +					"Failed to power on device: %pe\n",
> +					ERR_PTR(ret));
> +				for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +					pci_pwrctrl_destroy_device(of_port);
> +				}
> +			}
> +			return ret;
> +		}
> +	}
> +
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = apple_pcie_setup_port(pcie, of_port);
>  		if (ret) {

[Severity: Medium]
What happens to already-enabled port regulators if a subsequent port fails or
defers in apple_pcie_init()?

Once an earlier port succeeds in pci_pwrctrl_power_on_device(), its regulator
has been enabled via slot_pwrctrl_power_on().

1. If a later port returns -EPROBE_DEFER from pci_pwrctrl_power_on_device(),
   apple_pcie_init() returns without calling pci_pwrctrl_power_off_device() on
   the ports that were already powered on. On the next probe retry,
   pci_pwrctrl_power_on_device() will enable those regulators a second time,
   leaking regulator enable reference counts.

2. If pci_pwrctrl_power_on_device() fails with an error other than
   -EPROBE_DEFER, apple_pcie_init() calls pci_pwrctrl_destroy_device() on all
   ports without calling pci_pwrctrl_power_off_device() first, which unbinds
   the slot pwrctrl device and runs devm_slot_pwrctrl_release() ->
   regulator_bulk_free() while the regulator is still enabled, triggering
   WARN_ON(regulator->enable_count) in _regulator_put().

3. If pci_pwrctrl_create_device() fails on a later port or
   apple_pcie_setup_port() fails in the second loop, the previously created
   and powered-on pwrctrl devices are neither powered off nor destroyed.

Could apple_pcie_init() use pci_pwrctrl_create_devices() and
pci_pwrctrl_power_on_devices() (which already unwind previously powered-on or
created child nodes on error), and call pci_pwrctrl_power_off_devices() and
pci_pwrctrl_destroy_devices() if apple_pcie_setup_port() fails?

> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 0e2387cf46577..b59dbedf251bc 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c
> @@ -191,7 +191,16 @@ static int __pci_pwrctrl_power_off_device(struct device *dev)
>  	return pwrctrl->power_off(pwrctrl);
>  }
>  
> -static void pci_pwrctrl_power_off_device(struct device_node *np)
> +/**
> + * pci_pwrctrl_power_off_device - Power off pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
> + * Recursively traverse all pwrctrl devices for the devicetree hierarchy
> + * below the specified PCI port or device and power them off in a depth
> + * first manner.
> + */
> +void pci_pwrctrl_power_off_device(struct device_node *np)

[ ... ]

> @@ -245,11 +255,21 @@ static int __pci_pwrctrl_power_on_device(struct device *dev)
>  	return pwrctrl->power_on(pwrctrl);
>  }
>  
> -/*
> +/**
> + * pci_pwrctrl_power_on_device - Power on pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
>   * Power on the devices in a depth first manner. Before powering on the device,
>   * make sure its driver is bound.
> + * Recursively traverse all pwrctrl devices for the devicetree hierarchy
> + * below the specified PCI port or device and power them on in a depth
> + * first manner.  Before powering on the device, make sure its driver is bound.
> + *
> + * Return: 0 on success, -EPROBE_DEFER if any pwrctrl driver is not bound, an
> + * appropriate error code otherwise.
>   */
> -static int pci_pwrctrl_power_on_device(struct device_node *np)
> +int pci_pwrctrl_power_on_device(struct device_node *np)

[ ... ]

> @@ -335,7 +356,17 @@ int pci_pwrctrl_power_on_devices(struct device *parent)
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_power_on_devices);
>  
> -static int pci_pwrctrl_create_device(struct device_node *np,
> +/**
> + * pci_pwrctrl_create_device - Create pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
> + * Recursively create pwrctrl devices for the devicetree hierarchy below
> + * the specified PCI port or device in a depth first manner.
> + *
> + * Return: 0 on success, negative error number on error.
> + */
> +int pci_pwrctrl_create_device(struct device_node *np,
>  				     struct device *parent)

[ ... ]

> @@ -396,7 +428,15 @@ int pci_pwrctrl_create_devices(struct device *parent)
>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_create_devices);
>  
> -static void pci_pwrctrl_destroy_device(struct device_node *np)
> +/**
> + * pci_pwrctrl_destroy_device - Destroy pwrctrl devices
> + *
> + * @device_node: PCI port or device to operate on
> + *
> + * Recursively destroy pwrctrl devices for the devicetree hierarchy below
> + * the specified PCI port or device in a depth first manner.
> + */
> +void pci_pwrctrl_destroy_device(struct device_node *np)

[Severity: Low]
In these four kernel-doc comments in drivers/pci/pwrctrl/core.c, the parameter
is documented as @device_node instead of @np, pci_pwrctrl_create_device() is
missing @parent, and pci_pwrctrl_power_on_device() repeats "Before powering on
the device, make sure its driver is bound." twice. Could these be updated to
avoid W=1 kernel-doc warnings?

[ ... ]

> diff --git a/include/linux/pci-pwrctrl.h b/include/linux/pci-pwrctrl.h
> index 1192a2527521d..f27be6489b50f 100644
> --- a/include/linux/pci-pwrctrl.h
> +++ b/include/linux/pci-pwrctrl.h
> @@ -59,6 +59,10 @@ int pci_pwrctrl_create_devices(struct device *parent);
>  void pci_pwrctrl_destroy_devices(struct device *parent);
>  int pci_pwrctrl_power_on_devices(struct device *parent);
>  void pci_pwrctrl_power_off_devices(struct device *parent);
> +int pci_pwrctrl_create_device(struct device_node *np, struct device *parent);
> +void pci_pwrctrl_destroy_device(struct device_node *np);
> +int pci_pwrctrl_power_on_device(struct device_node *np);
> +void pci_pwrctrl_power_off_device(struct device_node *np);
>  #else
>  static inline int pci_pwrctrl_create_devices(struct device *parent) { return 0; }
>  static void pci_pwrctrl_destroy_devices(struct device *parent) { }

[Severity: Low]
Should include/linux/pci-pwrctrl.h also provide static inline stubs in the
#else (!CONFIG_PCI_PWRCTRL) branch for pci_pwrctrl_create_device(),
pci_pwrctrl_destroy_device(), pci_pwrctrl_power_on_device(), and
pci_pwrctrl_power_off_device(), along with a struct device_node forward
declaration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-apple-pcie-pwren-v2-0-216a4066965f@cyberchaos.dev?part=3