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

[Dilip Kota <eswara.kota@linux.intel.com>]

On 2/19/2020 10:42 PM, Rob Herring wrote:
> 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.
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

>