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;
From:   Dilip Kota <eswara.kota@linux.intel.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel
 Combophy
To:     Rob Herring <robh@kernel.org>
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
References: <208fcb9660abd560aeab077442d158d84a3dddee.1582021248.git.eswara.kota@linux.intel.com>
 <CAL_JsqL56Ucpm2FK4BPKS+N=5Zxn1iQht7OdJK1pE7cPxtWL-w@mail.gmail.com>
Message-ID: <6f4cad61-b094-78e5-9921-9de0a1d7dbd7@linux.intel.com>
Date:   Thu, 20 Feb 2020 17:58:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.1
MIME-Version: 1.0
In-Reply-To: <CAL_JsqL56Ucpm2FK4BPKS+N=5Zxn1iQht7OdJK1pE7cPxtWL-w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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

>