Re: [PATCH v5 1/2] dt-bindings: phy: airoha: Document support for AN7583 Gen3 PCIe PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: phy: airoha: Gen2 QPHY modeled as sub-PHY of Gen3 PHY
- [Medium] dt-bindings: phy: airoha: single 4-byte registers modeled as reg blocks
--

commit e8f61fa61bb7ae6492d5b28847c5a694f3cca2ec
Author: Christian Marangi <ansuelsmth@gmail.com>

dt-bindings: phy: airoha: Document support for AN7583 Gen3 PCIe PHY

This commit adds the Device Tree binding schema for the Airoha AN7583 Gen3
PCIe PHY, defining its register regions and QPHY phandle link.

> diff --git a/Documentation/devicetree/bindings/phy/airoha,an7583-pcie-gen3-phy.yaml b/Documentation/devicetree/bindings/phy/airoha,an7583-pcie-gen3-phy.yaml
> new file mode 100644
> index 0000000000000..a8ec5ac781ccc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/airoha,an7583-pcie-gen3-phy.yaml
> @@ -0,0 +1,65 @@

[ ... ]

> +  reg:
> +    items:
> +      - description: PCIE G3 analog base address
> +      - description: PCIE G3 PMA base address
> +      - description: PCIE detection time base address
> +      - description: PCIE Rx AEQ base address
> +
> +  reg-names:
> +    items:
> +      - const: g3-ana
> +      - const: g3-pma
> +      - const: xr-dtime
> +      - const: rx-aeq
> +
> +  phys:
> +    items:
> +      - description: phandle to QPHY phy for Gen2 support
> +
> +  "#phy-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - phys
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/phy/phy.h>
> +
> +    phy@1fc7f000 {
> +        compatible = "airoha,an7583-pcie-gen3-phy";
> +        reg = <0x1fc7f000 0x1000>,
> +              <0x1fc7e000 0x1000>,
> +              <0x1fc30044 0x4>,
> +              <0x1fc35030 0x4>;
> +        reg-names = "g3-ana", "g3-pma",
> +                    "xr-dtime", "rx-aeq";

[Severity: Medium]
As reported by Krzysztof Kozlowski on v2 [1], should the reg and reg-names
properties describe the enclosing hardware register blocks rather than
mapping two individual 4-byte registers for xr-dtime (0x1fc30044, size 0x4)
and rx-aeq (0x1fc35030, size 0x4)?

In the companion driver header drivers/phy/airoha/phy-an7583-pcie-regs.h,
these regions are accessed at offset 0x0 via REG_PCIE_PEXTP_DIG_GLB44 and
REG_PCIE_PEXTP_DIG_LN_RX30:

    /* Stone Portrait XR DTIME */
    #define REG_PCIE_PEXTP_DIG_GLB44		0x0

    /* Stone Portrait RX AEQ */
    #define REG_PCIE_PEXTP_DIG_LN_RX30		0x0

Consistent with the MediaTek and Airoha PEXTP register layout in
drivers/phy/mediatek/phy-mtk-pcie-sphy.c and
drivers/phy/phy-airoha-pcie-regs.h, 0x1fc30044 is offset 0x44 within the
PEXTP_DIG_GLB block at 0x1fc30000, and 0x1fc35030 is offset 0x30 within the
PEXTP_DIG_LN_RX block at 0x1fc35000 (offset 0x5030 from 0x1fc30000).

Mapping only 4 bytes per register in the binding prevents accessing any
other registers in those PEXTP blocks without changing the binding ABI.

> +
> +        phys = <&pcie1_phy PHY_TYPE_PCIE>;

[Severity: Medium]
As reported by Rob Herring on v3 [2], why is the Gen2 QPHY linked as a
required phys property on the Gen3 PHY node instead of referencing both the
Gen2 and Gen3 PHYs from the PCIe host controller?

The commit message states that QPHY setup must be done in the PCIe PHY
setup and calibration phase or calibration might fail. However, looking at
an7583_pcie_phy_init() and an7583_pcie_phy_exit() in the companion driver
(drivers/phy/airoha/phy-an7583-pcie.c), there is no interleaved register
access or cross-calibration between the two PHYs:

    static int an7583_pcie_phy_init(struct phy *phy)
    {
        ...
        an7583_pcie_phy_init_default(pcie_phy);
        ...
        setbits(pcie_phy->g3_pma + REG_SS_DA_XPON_PWDB_0,
                G3_PMA_DA_XPON_CDR_PR_PWDB);

        ret = phy_init(pcie_phy->qp_phy);
        if (ret)
                return ret;

        msleep(PHY_HW_INIT_TIME_MS);

        return 0;
    }

    static int an7583_pcie_phy_exit(struct phy *phy)
    {
        ...
        ret = phy_exit(pcie_phy->qp_phy);
        if (ret)
                return ret;

        an7583_pcie_phy_deinit(pcie_phy);

        return 0;
    }

In addition, an7583_pcie_phy_probe() retrieves qp_phy using
devm_phy_optional_get() even though phys is marked required in this binding
schema. Could both PHYs be linked directly to the PCIe host controller
instead of nesting phys inside the Gen3 PHY binding?

[1] https://lore.kernel.org/r/20260831-elastic-turquoise-puma-f032fc@quoll
[2] https://lore.kernel.org/r/20260917215107.GA3884839-robh@kernel.org

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy