Re: [PATCH v1 1/3] dt-bindings: phy: Add i.MX8QM PCIe PHY binding

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/08/2023 08:45, Richard Zhu wrote:
> Add i.MX8QM PCIe PHY binding.
> 
> i.MX8QM HSIO(High Speed IO) module has three instances of single lane
> SERDES PHYs, an instance of two lanes PCIe GEN3 controller, an
> instance of single lane PCIe GEN3 controller, as well as an instance
> of SATA 3.0 controller.
> 
> The HSIO module can be configured as the following different usecases.
> 1 - A two lanes PCIea and a single lane SATA.
> 2 - A single lane PCIea, a single lane PCIeb and a single lane SATA.
> 3 - A two lanes PCIea, a single lane PCIeb.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  .../bindings/phy/fsl,imx8-pcie-phy.yaml       | 70 ++++++++++++++++++-
>  1 file changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
> index 182a219387b0..764790f2b10b 100644
> --- a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
> @@ -17,16 +17,18 @@ properties:
>      enum:
>        - fsl,imx8mm-pcie-phy
>        - fsl,imx8mp-pcie-phy
> +      - fsl,imx8qm-pcie-phy
>  
>    reg:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 5
>  
>    clock-names:
> -    items:
> -      - const: ref
> +    minItems: 1
> +    maxItems: 5
>  
>    resets:
>      minItems: 1
> @@ -70,6 +72,36 @@ properties:
>      description: PCIe PHY  power domain (optional).
>      maxItems: 1
>  
> +  hsio-cfg:

Missing vendor prefix because it does not look like generic property.

> +    description: |
> +      Specifies the different usecases supported by the HSIO(High Speed IO)

I don't know what are the usecases...

> +      module. PCIEAX2SATA means two lanes PCIea and a single lane SATA.
> +      PCIEAX1PCIEBX1SATA represents a single lane PCIea, a single lane
> +      PCIeb and a single lane SATA. PCIEAX2PCIEBX1 on behalf of a two
> +      lanes PCIea, a single lane PCIeb.
> +      Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants to
> +      be used (optional).

None of all this helped me to understand what part of hardware this is
responsible for. It seems you just want to program a register, but
instead you should use one of existing properties like phy-modes etc.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 1, 2, 3 ]
> +
> +  ctrl-csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the ctrl-csr region containing the HSIO control and
> +      status registers for PCIe or SATA controller (optional).

Please try some internal review before posting to patches. Community is
not cheap reviewers taking this duty from NXP. I am pretty sure NXP can
afford someone looking at the code.

This misses vendor prefix, as explained many times for every syscon
phandle. Also optional is redundant.

But anyway status of PCIe or SATA controller is not a property of the
phy, so it looks to me you stuff here some properties belonging to some
other missing devices.

> +
> +  misc-csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the misc-csr region containing the HSIO control and
> +      status registers for misc (optional).

Same problems.

> +
> +  phy-csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the phy-csr region containing the HSIO control and
> +      status registers for phy (optional).

Same problems.


> +
>  required:
>    - "#phy-cells"
>    - compatible
> @@ -78,6 +110,38 @@ required:
>    - clock-names
>    - fsl,refclk-pad-mode
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - fsl,imx8qm-pcie-phy
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 4
> +          maxItems: 5
> +        clock-names:
> +          oneOf:
> +            - items:
> +                - const: pipe_pclk
> +                - const: ctrl_ips_clk
> +                - const: phy_ips_clk
> +                - const: misc_ips_clk

Drop clk everywhere.
> +            - items:
> +                - const: apb_pclk

No, optional clock goes to the end and please explain why APB is optional.



Best regards,
Krzysztof