Re: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Error handling in `ti_pipe3_acspcie_tx_rx_mode` breaks probe deferral and causes warning spam on older device trees
- [Low] Schema Validation Incompleteness: `syscon-acspcie-tx-rx` allows invalid device tree configurations
--

--- Patch [2]: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting ---
commit c7627fd3448f76b00b303ca95c47a9daed73d133
Author: David Oberhollenzer <[email protected]>

dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting
    
Add syscon-acspcie-tx-rx property to the PHY bindings.

> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> index 84f538aa587c4..4fb017df28985 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> @@ -52,6 +52,15 @@ properties:
>        enum: [wkupclk, sysclk, refclk, dpll_ref,
>               dpll_ref_m2, phy-div, div-clk]
>  
> +  syscon-acspcie-tx-rx:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    maxItems: 1
> +    items:
> +      items:
> +        - description: Phandle to the system control module
> +        - description: Register offset of CTRL_CORE_SMA_SW_6
> +        - description: Setting for PCIE_TX_RX_CONTROL
> +

[Severity: Low]
Does this allow invalid device tree configurations to pass validation?

Because this is defined in the top-level properties block and lacks an allOf
conditional if-then block, schema validation (dt_binding_check) will silently
pass if this PCIe-specific property is incorrectly added to SATA or USB3 PHY
nodes.

Should this property be restricted exclusively to the ti,phy-pipe3-pcie
compatible string?

>    syscon-phy-power:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1

Also, looking at the corresponding driver changes that implement this new
property in drivers/phy/ti/phy-ti-pipe3.c:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_acspcie_tx_rx_mode() {
        ...
        regmap = syscon_regmap_lookup_by_phandle_args(np,
                                                      "syscon-acspcie-tx-rx",
                                                      2, args);
        if (IS_ERR(regmap)) {
                dev_warn(phy->dev, "can't get syscon-acspcie-tx-rx\n");
                return -EINVAL;
        }
        ...
}

[Severity: High]
Does this error handling break probe deferral?

If the syscon provider is not yet ready,
syscon_regmap_lookup_by_phandle_args() returns -EPROBE_DEFER, which is
converted to -EINVAL here.

Additionally, if the syscon-acspcie-tx-rx property is missing (which is
expected on older valid device trees), it returns -ENOENT, leading to
warning spam.

In the caller:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_probe() {
        ...
        if (phy->mode == PIPE3_MODE_PCIE) {
                ret = ti_pipe3_acspcie_tx_rx_mode(phy);
                if (ret)
                        dev_warn(dev, "failed to set ACSPCIe TX/RX mode\n");
        }
        ...
}

Because ti_pipe3_probe() ignores the error code, does this mean probe
succeeds even if the syscon provider deferred?

This could lead to silent hardware initialization failure on systems where
syscon is deferred.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2