Re: [PATCH v4 2/3] dt-bindings: phy: Add YAML schemas for Intel Combophy

[Rob Herring <robh@kernel.org>]

On Mon, Mar 02, 2020 at 04:43:24PM +0800, Dilip Kota 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 v4:
>   No changes.
> 
> Changes on v3:
> 
>  Add include/dt-bindings/phy/phy-intel-combphy.h for phy modes.
>  Add SoC specific compatible "intel,combophy-lgm".
>  Correct the nodename pattern.
>  clocks description removed and add maxItems entry.
>  Remove "simple-bus" as it expects minimum one address
>   cell and size cell in the children node. Call devm_of_platform_populate()
>   in the driver to perform "simple-bus" functionality.
> 
> 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   | 123 +++++++++++++++++++++
>  include/dt-bindings/phy/phy-intel-combophy.h       |  10 ++
>  2 files changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>  create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h
> 
> 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..f9bae37fab17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> @@ -0,0 +1,123 @@
> +# 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.
> +
> +properties:
> +  $nodename:
> +    pattern: "combophy(@.*|-[0-9a-f])*$"
> +
> +  compatible:
> +    items:
> +      - const: intel,combophy-lgm
> +      - const: intel,combo-phy
> +
> +  clocks:
> +    maxItems: 1
> +
> +  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-array
> +    description: Chip configuration registers handle and ComboPhy instance id
> +
> +  intel,hsio:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: HSIO registers handle and ComboPhy instance id on NOC
> +
> +  intel,aggregation:
> +    type: boolean
> +    description: |
> +      Specify the flag to configure ComboPHY in dual lane mode.
> +
> +  intel,phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Mode of the two phys in ComboPhy.
> +      See dt-bindings/phy/phy-intel-combophy.h for values.
> +
> +patternProperties:
> +  "^phy@[0-9]+$":
> +    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:
> +  - |
> +    #include <dt-bindings/phy/phy-intel-combophy.h>
> +    combophy@d0a00000 {
> +        compatible = "intel,combophy-lgm", "intel,combo-phy";
> +        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 = <COMBO_PHY_PCIE>;
> +
> +        phy@0 {

You need a 'reg' property to go with a unit-address.

Really, I'd just simplify this to make parent 'resets' be 4 entries and 
put '#phy-cells = <1>;' in the parent. Then you don't need these child 
nodes.

> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 23>;
> +        };
> +
> +        phy@1 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 24>;
> +        };
> +    };
> diff --git a/include/dt-bindings/phy/phy-intel-combophy.h b/include/dt-bindings/phy/phy-intel-combophy.h
> new file mode 100644
> index 000000000000..bd7f6377f4ef
> --- /dev/null
> +++ b/include/dt-bindings/phy/phy-intel-combophy.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _DT_BINDINGS_INTEL_COMBOPHY
> +#define _DT_BINDINGS_INTEL_COMBOPHY
> +
> +#define COMBO_PHY_PCIE	0
> +#define COMBO_PHY_XPCS	1
> +#define COMBO_PHY_SATA	2

Use the PHY_TYPE_* defines we already have and extend as you need to.

> +
> +#endif /* _DT_BINDINGS_INTEL_COMBOPHY*/
> -- 
> 2.11.0
>