Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Kishon Vijay Abraham I <kishon@ti.com>
Subject: Re: [PATCH 1/9] dt-bindings: phy: cadence-sierra: Add bindings for
 the PLLs within SERDES
To:     Rob Herring <robh@kernel.org>
CC:     Vinod Koul <vkoul@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Swapnil Kashinath Jakhade <sjakhade@cadence.com>,
        Milind Parab <mparab@cadence.com>,
        Yuti Suresh Amonkar <yamonkar@cadence.com>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>
References: <20201103035556.21260-1-kishon@ti.com>
 <20201103035556.21260-2-kishon@ti.com> <20201105180308.GA1540220@bogus>
Message-ID: <c3146272-8108-7f12-f465-f6c5c7556112@ti.com>
Date:   Mon, 21 Dec 2020 08:40:36 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201105180308.GA1540220@bogus>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Rob,

On 05/11/20 11:33 pm, Rob Herring wrote:
> On Tue, Nov 03, 2020 at 09:25:48AM +0530, Kishon Vijay Abraham I wrote:
>> Add binding for the PLLs within SERDES.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/phy/phy-cadence-sierra.yaml      | 89 ++++++++++++++++++-
>>  1 file changed, 86 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
>> index d210843863df..f574b8ed358c 100644
>> --- a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
>> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
>> @@ -49,12 +49,14 @@ properties:
>>      const: serdes
>>  
>>    clocks:
>> -    maxItems: 2
>> +    maxItems: 4
>>  
>>    clock-names:
>>      items:
>>        - const: cmn_refclk_dig_div
>>        - const: cmn_refclk1_dig_div
>> +      - const: pll_cmnlc
>> +      - const: pll_cmnlc1
>>  
>>    cdns,autoconf:
>>      type: boolean
>> @@ -107,6 +109,58 @@ patternProperties:
>>  
>>      additionalProperties: false
>>  
>> +  "^refrcv1?$":
>> +    type: object
>> +    description: |
>> +      Reference receivers that enables routing external clocks to the alternate
>> +      PLLCMNLC.
>> +    properties:
>> +      clocks:
>> +        maxItems: 1
>> +        description: Phandle to clock nodes representing the input to the
>> +          reference receiver.
>> +
>> +      clock-names:
>> +        items:
>> +          - const: pll_refclk
>> +
>> +      "#clock-cells":
>> +        const: 0
>> +
>> +    required:
>> +      - clocks
>> +      - "#clock-cells"
>> +
>> +  "^pll_cmnlc1?$":
>> +    type: object
>> +    description: |
>> +      SERDES node should have subnodes for each of the PLLs present in
>> +      the SERDES.
>> +    properties:
>> +      clocks:
>> +        maxItems: 2
>> +        description: Phandle to clock nodes representing the two inputs to PLL.
>> +
>> +      clock-names:
>> +        items:
>> +          - const: pll_refclk
>> +          - const: refrcv
>> +
>> +      "#clock-cells":
>> +        const: 0
>> +
>> +      assigned-clocks:
>> +        maxItems: 1
>> +
>> +      assigned-clock-parents:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - clocks
>> +      - "#clock-cells"
>> +      - assigned-clocks
>> +      - assigned-clock-parents
>> +
>>  required:
>>    - compatible
>>    - "#address-cells"
>> @@ -130,10 +184,39 @@ examples:
>>              reg = <0x0 0xfd240000 0x0 0x40000>;
>>              resets = <&phyrst 0>, <&phyrst 1>;
>>              reset-names = "sierra_reset", "sierra_apb";
>> -            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>;
>> -            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div";
>> +            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>, <&serdes_pll_cmnlc>, <&serdes_pll_cmnlc1>;
>> +            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div", "pll_cmnlc", "pll_cmnlc1";
>>              #address-cells = <1>;
>>              #size-cells = <0>;
>> +
>> +            serdes_refrcv: refrcv {
>> +                    clocks = <&pll0_refclk>;
>> +                    clock-names = "pll_refclk";
>> +                    #clock-cells = <0>;
>> +            };
>> +
>> +            serdes_refrcv1: refrcv1 {
>> +                    clocks = <&pll1_refclk>;
>> +                    clock-names = "pll_refclk";
>> +                    #clock-cells = <0>;
>> +            };
>> +
>> +            serdes_pll_cmnlc: pll_cmnlc {
>> +                    clocks = <&pll0_refclk>, <&serdes_refrcv1>;
>> +                    clock-names = "pll_refclk", "refrcv";
>> +                    #clock-cells = <0>;
>> +                    assigned-clocks = <&serdes_pll_cmnlc>;
> 
> Isn't assigned-clocks supposed to be one of the clocks in 'clocks'?
> 
>> +                    assigned-clock-parents = <&pll0_refclk>;
> 
> And this should not be a clock in 'clocks'...
> 
> 
> More generally, why do we need to expose all these details in DT?

Sierra serdes is highly configurable w.r.t which clock can be used for
its internal PLL. The Same SoC, depending on how it is configured in the
EVM can either use internal clock or external clock. In order to
flexible support all the options, have to expose these in DT.

Thank You,
Kishon