Re: [PATCH v2 13/14] dt-bindings: phy: Document WIZ (SERDES wrapper) bindings

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On 30/10/19 12:38 AM, Rob Herring wrote:
> On Wed, Oct 23, 2019 at 06:27:34PM +0530, Kishon Vijay Abraham I wrote:
>> Add DT binding documentation for WIZ (SERDES wrapper). WIZ is *NOT* a
>> PHY but a wrapper used to configure some of the input signals to the
>> SERDES. It is used with both Sierra(16G) and Torrent(10G) serdes.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> [jsarha@ti.com: Add separate compatible for Sierra(16G) and Torrent(10G)
>>  SERDES]
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  .../bindings/phy/ti,phy-j721e-wiz.yaml        | 159 ++++++++++++++++++
>>  1 file changed, 159 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml b/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
>> new file mode 100644
>> index 000000000000..8a1eccee6c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-j721e-wiz.yaml
>> @@ -0,0 +1,159 @@
>> +# SPDX-License-Identifier: (GPL-2.0)
> 
> (GPL-2.0-only OR BSD-2-Clause) for new bindings please.
> 
>> +# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/phy/ti,phy-j721e-wiz.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: TI J721E WIZ (SERDES Wrapper)
>> +
>> +maintainers:
>> +  - Kishon Vijay Abraham I <kishon@ti.com>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - ti,j721e-wiz-16g
>> +              - ti,j721e-wiz-10g
> 
> You can drop oneOf and items.
> 
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 3
>> +    description: clock-specifier to represent input to the WIZ
>> +
>> +  clock-names:
>> +    items:
>> +      - const: fck
>> +      - const: core_ref_clk
>> +      - const: ext_ref_clk
>> +
>> +  num-lanes:
>> +    maxItems: 1
>> +    minimum: 1
>> +    maximum: 4
> 
> You've mixed array and scalar schema keywords. Drop maxItems.
> 
> Update dtschema and run 'make dt_binding_check'. We should catch that 
> now.

Sure.
> 
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 2
>> +
>> +  "#reset-cells":
>> +    const: 1
>> +
>> +  ranges: true
>> +
>> +  assigned-clocks:
>> +    maxItems: 2
>> +
>> +  assigned-clock-parents:
>> +    maxItems: 2
>> +
>> +patternProperties:
>> +  "^pll[0|1]_refclk$":
>> +    type: object
>> +    description: |
>> +      WIZ node should have subnodes for each of the PLLs present in
>> +      the SERDES.
>> +
>> +  "^cmn_refclk1?$":
>> +    type: object
>> +    description: |
>> +      WIZ node should have subnodes for each of the PMA common refclock
>> +      provided by the SERDES.
>> +
>> +  "^refclk_dig$":
>> +    type: object
>> +    description: |
>> +      WIZ node should have subnode for refclk_dig to select the reference
>> +      clock source for the reference clock used in the PHY and PMA digital
>> +      logic.
>> +
>> +  "^serdes@[0-9a-f]+$":
>> +    type: object
>> +    description: |
>> +      WIZ node should have '1' subnode for the SERDES. It could be either
>> +      Sierra SERDES or Torrent SERDES. Sierra SERDES should follow the
>> +      bindings specified in
>> +      Documentation/devicetree/bindings/phy/phy-cadence-sierra.txt
>> +      Torrent SERDES should follow the bindings specified in
>> +      Documentation/devicetree/bindings/phy/phy-cadence-dp.txt
>> +
>> +required:
>> +  - compatible
>> +  - power-domains
>> +  - clocks
>> +  - clock-names
>> +  - num-lanes
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - "#reset-cells"
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
>> +
>> +    wiz@5000000 {
>> +           compatible = "ti,j721e-wiz-16g";
>> +           #address-cells = <2>;
>> +           #size-cells = <2>;
> 
> Really need 64-bits of address space for the child nodes?

hmm, the register space for the child nodes are in the 32-bit address space
region. I'll fix this.
> 
>> +           power-domains = <&k3_pds 292 TI_SCI_PD_EXCLUSIVE>;
>> +           clocks = <&k3_clks 292 5>, <&k3_clks 292 11>, <&dummy_cmn_refclk>;
>> +           clock-names = "fck", "core_ref_clk", "ext_ref_clk";
>> +           assigned-clocks = <&k3_clks 292 11>, <&k3_clks 292 0>;
>> +           assigned-clock-parents = <&k3_clks 292 15>, <&k3_clks 292 4>;
>> +           num-lanes = <2>;
>> +           #reset-cells = <1>;
> 
> Unless you have additional registers, I'm not a fan of wrapper nodes.

The wrapper node has TI specific registers while the child node has Cadence
Sierra specific registers. It also has clock nodes which are input to the
Sierra IP.
> 
>> +
>> +           pll0_refclk {
>> +                  clocks = <&k3_clks 293 13>, <&dummy_cmn_refclk>;
>> +                  clock-output-names = "wiz1_pll0_refclk";
>> +                  #clock-cells = <0>;
>> +                  assigned-clocks = <&wiz1_pll0_refclk>;
>> +                  assigned-clock-parents = <&k3_clks 293 13>;
>> +           };
>> +
>> +           pll1_refclk {
>> +                  clocks = <&k3_clks 293 0>, <&dummy_cmn_refclk1>;
>> +                  clock-output-names = "wiz1_pll1_refclk";
>> +                  #clock-cells = <0>;
>> +                  assigned-clocks = <&wiz1_pll1_refclk>;
>> +                  assigned-clock-parents = <&k3_clks 293 0>;
>> +           };
>> +
>> +           cmn_refclk {
>> +                  clocks = <&wiz1_refclk_dig>;
>> +                  clock-output-names = "wiz1_cmn_refclk";
>> +                  #clock-cells = <0>;
>> +           };
>> +
>> +           cmn_refclk1 {
>> +                  clocks = <&wiz1_pll1_refclk>;
>> +                  clock-output-names = "wiz1_cmn_refclk1";
>> +                  #clock-cells = <0>;
>> +           };
>> +
>> +           refclk_dig {
>> +                  clocks = <&k3_clks 292 11>, <&k3_clks 292 0>, <&dummy_cmn_refclk>, <&dummy_cmn_refclk1>;
>> +                  clock-output-names = "wiz0_refclk_dig";
>> +                  #clock-cells = <0>;
>> +                  assigned-clocks = <&wiz0_refclk_dig>;
>> +                  assigned-clock-parents = <&k3_clks 292 11>;
>> +           };
> 
> How are all these clocks programmed?

All these are programmed in the WIZ driver which is implemented in 14/14 of
this series.

Thanks
Kishon