Re: [PATCH v5 1/2] dt-bindings: PCI: rockchip,rk3399-pcie: Add PERST# inactive delay properties

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: PCI: rockchip,rk3399-pcie: OS policy and magic 0 in PERST# props
--

commit 9f3338157ea2855233568eb255bd2687fadd36fe
Author: Enrique Hernández Bello <ehbello@gmail.com>

dt-bindings: PCI: rockchip,rk3399-pcie: Add PERST# inactive delay properties

This commit adds the optional rockchip,perst-inactive-ms and
rockchip,s2r-perst-inactive-ms properties to the RK3399 PCIe host controller
binding to configure the reset assertion delay at probe and system resume.

> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> index 720a5f945a4e6..7ac915c267910 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> @@ -40,6 +40,21 @@ properties:
>      maxItems: 1
>      description: pre-reset GPIO
>  
> +  rockchip,perst-inactive-ms:
> +    default: 100
> +    description:
> +      Time PERST# is held asserted before it is released, which is also the
> +      width of the reset pulse. The default meets T_PVPERL for a slot that
> +      is powered at the moment PERST# is asserted. Some endpoints need more
> +      settling time than that, while others stop responding when the pulse
> +      is that long. A value of 0 selects the 100 us minimum the PCI Express
> +      CEM specification sets for the pulse.
> +
> +  rockchip,s2r-perst-inactive-ms:
> +    description:
> +      The same time, applied when resuming from system suspend. Defaults to
> +      the value of rockchip,perst-inactive-ms.

[Severity: Medium]
Are vendor-specific host controller properties split by OS lifecycle state
the right way to model this in Device Tree?

Splitting the reset delay into rockchip,perst-inactive-ms (probe) and
rockchip,s2r-perst-inactive-ms (system suspend-to-ram resume) encodes OS
driver power-management transitions directly into the binding rather than
describing hardware characteristics.

In drivers/pci/controller/pcie-rockchip-host.c, rockchip_pcie_suspend_noirq()
only disables vpcie0v9 while leaving vpcie12v, vpcie3v3, and vpcie1v8
enabled, and does not assert perst_gpio on suspend. Then on both probe and
resume, rockchip_pcie_host_init_port() asserts perst_gpio, enables link
training (PCIE_CLIENT_LINK_TRAIN_ENABLE) while reset is still asserted, and
sleeps for PCIE_T_PVPERL_MS (100 ms) before deasserting perst_gpio.

Holding reset asserted for 100 ms after enabling link training or when slot
power rails are already stable looks like a sequencing issue in
pcie-rockchip-host.c rather than a property of the RK3399 controller.

Also, if an endpoint requires a non-standard reset delay, shouldn't that be
described using generic reset or PCI device properties (such as
reset-assert-ms or reset-delay-us) rather than SoC-specific host properties?
Neither rockchip,perst-inactive-ms nor rockchip,s2r-perst-inactive-ms exists
in mainline Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml.

Finally, rockchip,perst-inactive-ms uses the standard -ms unit suffix, but
defines 0 as a magic value that selects a 100 us pulse width instead of 0 ms,
and names the active-low reset assertion duration "perst-inactive-ms" even
though the signal is held active (asserted) during this window. Could this be
handled by fixing the driver reset sequencing or using standard unit-accurate
properties instead?

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