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

[Dilip Kota]

On 2/19/2020 10:42 PM, Rob Herring wrote:
> On Tue, Feb 18, 2020 at 9:31 PM Dilip Kota<eswara.kota@xxxxxxxxxxxxxxx>  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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
> > +
> > +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.
Will fix it.
> > +
> > +  compatible:
> > +    items:
> > +      - const: intel,combo-phy
> Needs to be an SoC specific compatible.
Sure, will update it to intel, combophy-lgm
> > +      - 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?
Its only one clock, i will add maxItems:1 and remove the description.
> > +
> > +  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
Sure, will define the types in header file.
> 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.
Two PHYs must be in same mode.
actual mode configuration is done for the ComboPhy to work as a two 
individual PHYs in PCIe/XPCS/SATA mode or as a single PHY providing dual 
physical lanes in PCIe/XPCS.
And mode configuration is dependent on the 'intel,aggregation' flag.
So, placed the phy-mode here itself, to make sure all the mode related 
parameters are at one location. Also, avoids setting individual PHYs in 
different modes.

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

Sure, will fix it.

Regards,
Dilip

>