Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <208fcb9660abd560aeab077442d158d84a3dddee.1582021248.git.eswara.kota@linux.intel.com>
In-Reply-To: <208fcb9660abd560aeab077442d158d84a3dddee.1582021248.git.eswara.kota@linux.intel.com>
From:   Rob Herring <robh@kernel.org>
Date:   Wed, 19 Feb 2020 08:42:13 -0600
Message-ID: <CAL_JsqL56Ucpm2FK4BPKS+N=5Zxn1iQht7OdJK1pE7cPxtWL-w@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy
To:     Dilip Kota <eswara.kota@linux.intel.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        devicetree@vger.kernel.org, Kishon Vijay Abraham I <kishon@ti.com>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        cheol.yong.kim@intel.com, chuanhua.lei@linux.intel.com,
        qi-ming.wu@intel.com, yixin.zhu@intel.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 18, 2020 at 9:31 PM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>
> Combophy subsystem provides PHY support to various
> controllers, viz. PCIe, SATA and EMAC.
> Adding YAML schemas for the same.
>
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
> Changes on v2:
>
>  Add custom 'select'
>  Pass hardware instance entries with phandles and
>    remove cell-index and bid entries
>  Clock, register address space, are same for the children.
>    So move them to parent node.
>  Two PHY instances cannot run in different modes,
>    so move the phy-mode entry to parent node.
>  Add second child entry in the DT example.
>
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> new file mode 100644
> index 000000000000..8e65a2a71e7f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> @@ -0,0 +1,138 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Combophy Subsystem
> +
> +maintainers:
> +  - Dilip Kota <eswara.kota@linux.intel.com>
> +
> +description: |
> +  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
> +  controllers. A single Combophy provides two PHY instances.
> +
> +# We need a select here so we don't match all nodes with 'simple-bus'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: intel,combo-phy
> +  required:
> +    - compatible
> +
> +properties:
> +  $nodename:
> +    pattern: "^combophy@[0-9]+$"

Unit addresses are hex.

> +
> +  compatible:
> +    items:
> +      - const: intel,combo-phy

Needs to be an SoC specific compatible.

> +      - const: simple-bus
> +
> +  clocks:
> +    description: |
> +      List of phandle and clock specifier pairs as listed
> +      in clock-names property. Configure the clocks according
> +      to the PHY mode.

How many?

No need to redefine a common property name, drop description. Plus,
where's clock-names?

> +
> +  reg:
> +    items:
> +      - description: ComboPhy core registers
> +      - description: PCIe app core control registers
> +
> +  reg-names:
> +    items:
> +      - const: core
> +      - const: app
> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: phy
> +      - const: core
> +
> +  intel,syscfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Chip configuration registers handle and ComboPhy instance id
> +
> +  intel,hsio:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: HSIO registers handle and ComboPhy instance id on NOC
> +
> +  intel,aggregation:
> +    description: |
> +      Specify the flag to confiure ComboPHY in dual lane mode.
> +    type: boolean
> +
> +  intel,phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2
> +    description: |
> +      Configure the mode of the PHY.
> +        0 - PCIe
> +        1 - xpcs
> +        2 - sata

Doesn't this need to be per PHY? Or the 2 PHYs have to be the same mode?

Use the types defined in include/dt-bindings/phy/phy.h. You'll need to
add XPCS which maybe should be more specific to distinguish 1G, 10G,
etc. Also, we typically put the mode into the 'phys' cells so the mode
lives with the client node.

> +
> +patternProperties:
> +  "^cb[0-9]phy@[0-9]+$":

^phy@...

> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: intel,phydev
> +
> +      "#phy-cells":
> +        const: 0
> +
> +      resets:
> +        description: |
> +          reset handle according to the PHY mode.
> +          See ../reset/reset.txt for details.
> +
> +    required:
> +      - compatible
> +      - "#phy-cells"
> +
> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - reg-names
> +  - intel,syscfg
> +  - intel,hsio
> +  - intel,phy-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    combophy@0 {
> +        compatible = "intel,combo-phy", "simple-bus";
> +        clocks = <&cgu0 1>;
> +        reg = <0xd0a00000 0x40000>,
> +              <0xd0a40000 0x1000>;
> +        reg-names = "core", "app";
> +        resets = <&rcu0 0x50 6>,
> +                <&rcu0 0x50 17>;
> +        reset-names = "phy", "core";
> +        intel,syscfg = <&sysconf 0>;
> +        intel,hsio = <&hsiol 0>;
> +        intel,phy-mode = <0>;
> +
> +        cb0phy0:cb0phy@0 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 23>;
> +        };
> +
> +        cb0phy1:cb0phy@1 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 24>;
> +        };
> +    };
> --
> 2.11.0
>