linux-kernel - Re: [PATCH 1/9] dt-bindings: phy: cadence-sierra: Add bindings for the PLLs within SERDES

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Dec 2020 08:40:36 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Rob Herring <robh@...nel.org>
CC:     Vinod Koul <vkoul@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Swapnil Kashinath Jakhade <sjakhade@...ence.com>,
        Milind Parab <mparab@...ence.com>,
        Yuti Suresh Amonkar <yamonkar@...ence.com>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/9] dt-bindings: phy: cadence-sierra: Add bindings for
 the PLLs within SERDES

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@...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