Re: [RFC/RFT 1/6] dt-bindings: phy: rockchip: add pcie3 phy

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 16/04/2022 15:54, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> 
> Add a new binding file for Rockchip PCIe V3 phy driver.

Thank you for your patch. There is something to discuss/improve.

> 
> Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> ---
>  .../bindings/phy/rockchip-pcie3-phy.yaml      | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml
> new file mode 100644
> index 000000000000..58a8ce175f13
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/rockchip-pcie3-phy.yaml

Filename: vendor,hardware
so for example "rockchip,pcie3-phy" although Rob proposed recently for
other bindings using compatible as a base:
https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@xxxxxxxxxxxxxxxxxx/


> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/rockchip-pcie3-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip PCIe v3 phy
> +
> +maintainers:
> +  - Heiko Stuebner <heiko@xxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3568-pcie3-phy
> +      - rockchip,rk3588-pcie3-phy
> +
> +  reg:
> +    maxItems: 2
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  clock-names:
> +    contains:
> +      anyOf:
> +        - enum: [ refclk_m, refclk_n, pclk ]

The list should be strictly ordered (defined), so:
  items:
    - const: ...
    - const: ...
    - const: ...
  minItems: 1

However the question is - why the clocks have different amount? Is it
per different SoC implementation?

> +
> +  "#phy-cells":
> +    const: 0
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: phy
> +
> +  rockchip,phy-grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the syscon managing the phy "general register files"
> +
> +  rockchip,pipe-grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the syscon managing the pipe "general register files"
> +
> +  rockchip,pcie30-phymode:
> +    $ref: '/schemas/types.yaml#/definitions/uint32'
> +    description: |
> +      use PHY_MODE_PCIE_AGGREGATION if not defined

I don't understand the description. Do you mean here a case when the
variable is missing?

> +    minimum: 0x0
> +    maximum: 0x4

Please explain these values. Register values should not be part of
bindings, but instead some logical behavior of hardware or its logic.

> +
> +

Just one blank line.

> +required:
> +  - compatible
> +  - reg
> +  - rockchip,phy-grf

phy-cells as well

> +
> +additionalProperties: false
> +
> +unevaluatedProperties: false

Just one please, additionalProperties.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rk3568-cru.h>
> +    pcie30phy: phy@fe8c0000 {
> +      compatible = "rockchip,rk3568-pcie3-phy";
> +      reg = <0x0 0xfe8c0000 0x0 0x20000>;
> +      #phy-cells = <0>;
> +      clocks = <&pmucru CLK_PCIE30PHY_REF_M>, <&pmucru CLK_PCIE30PHY_REF_N>,
> +       <&cru PCLK_PCIE30PHY>;

Align the entry with opening '<'. Usually the most readable is one clock
per line.

> +      clock-names = "refclk_m", "refclk_n", "pclk";
> +      resets = <&cru SRST_PCIE30PHY>;
> +      reset-names = "phy";
> +      rockchip,phy-grf = <&pcie30_phy_grf>;
> +    };


Best regards,
Krzysztof