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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38e60bb2-123b-09cf-d6ef-3a07c6984108@linaro.org>
Date:   Mon, 18 Apr 2022 17:52:43 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Frank Wunderlich <linux@...web.de>,
        linux-rockchip@...ts.infradead.org
Cc:     Frank Wunderlich <frank-w@...lic-files.de>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Johan Jonker <jbx6244@...il.com>,
        Peter Geis <pgwipeout@...il.com>,
        Michael Riesch <michael.riesch@...fvision.net>,
        linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org
Subject: Re: [RFC/RFT 1/6] dt-bindings: phy: rockchip: add pcie3 phy

On 16/04/2022 15:54, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@...lic-files.de>
> 
> 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@...lic-files.de>
> ---
>  .../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@robh.at.kernel.org/


> @@ -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@...ech.de>
> +
> +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@...c0000 {
> +      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