[v6,1/3] dt-bindings: phy: Add DT bindings for Xilinx ZynqMP PSGTR PHY

From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>

Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
Processing System Gigabit Transceiver which provides PHY capabilities to
USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.

Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v5:

- Document clocks and clock-names properties
- Document resets and reset-names properties
- Replace subnodes with an additional entry in the PHY cells
- Drop lane frequency PHY cell, replaced by reference clock phandle
- Convert bindings to YAML
- Reword the subject line
- Drop Rob's R-b as the bindings have significantly changed
- Drop resets and reset-names properties
---
 .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
 include/dt-bindings/phy/phy.h                 |   1 +
 2 files changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml

+Rob

On 11/03/20 4:02 pm, Laurent Pinchart wrote:
> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> 
> Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> Processing System Gigabit Transceiver which provides PHY capabilities to
> USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> 
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v5:
> 
> - Document clocks and clock-names properties
> - Document resets and reset-names properties
> - Replace subnodes with an additional entry in the PHY cells
> - Drop lane frequency PHY cell, replaced by reference clock phandle
> - Convert bindings to YAML
> - Reword the subject line
> - Drop Rob's R-b as the bindings have significantly changed
> - Drop resets and reset-names properties
> ---
>  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
>  include/dt-bindings/phy/phy.h                 |   1 +
>  2 files changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> new file mode 100644
> index 000000000000..9948e4a60e45
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> @@ -0,0 +1,104 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |
> +  This binding describes the Xilinx ZynqMP Gigabit Transceiver (GTR) PHY. The
> +  GTR provides four lanes and is used by USB, SATA, PCIE, Display port and
> +  Ethernet SGMII controllers.
> +
> +properties:
> +  "#phy-cells":
> +    const: 4
> +    description: |
> +      The cells contain the following arguments.
> +
> +      - description: The GTR lane
> +        minimum: 0
> +        maximum: 3
> +      - description: The PHY type
> +        enum:
> +          - PHY_TYPE_DP
> +          - PHY_TYPE_PCIE
> +          - PHY_TYPE_SATA
> +          - PHY_TYPE_SGMII
> +          - PHY_TYPE_USB
> +      - description: The PHY instance
> +        minimum: 0
> +        maximum: 1 # for DP, SATA or USB
> +        maximum: 3 # for PCIE or SGMII
> +      - description: The reference clock number
> +        minimum: 0
> +        maximum: 3
> +
> +  compatible:
> +    enum:
> +      - xlnx,zynqmp-psgtr-v1.1
> +      - xlnx,zynqmp-psgtr
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 4
> +    description: |
> +      Clock for each PS_MGTREFCLK[0-3] reference clock input. Unconnected
> +      inputs shall not have an entry.
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      pattern: "^ref[0-3]$"
> +
> +  reg:
> +    items:
> +      - description: SERDES registers block
> +      - description: SIOU registers block
> +
> +  reg-names:
> +    items:
> +      - const: serdes
> +      - const: siou
> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - reg
> +  - reg-names
> +
> +if:
> +  properties:
> +    compatible:
> +      const: xlnx,zynqmp-psgtr
> +
> +then:
> +  properties:
> +    xlnx,tx-termination-fix:
> +      description: |
> +        Include this for fixing functional issue with the TX termination
> +        resistance in GT, which can be out of spec for the XCZU9EG silicon
> +        version.
> +      type: boolean
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    phy: phy@fd400000 {
> +      compatible = "xlnx,zynqmp-psgtr-v1.1";
> +      reg = <0x0 0xfd400000 0x0 0x40000>,
> +            <0x0 0xfd3d0000 0x0 0x1000>;
> +      reg-names = "serdes", "siou";
> +      clocks = <&refclks 3>, <&refclks 2>, <&refclks 0>;
> +      clock-names = "ref1", "ref2", "ref3";
> +      #phy-cells = <4>;
> +      status = "okay";
> +    };
> +
> +...
> diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
> index 1f3f866fae7b..f6bc83b66ae9 100644
> --- a/include/dt-bindings/phy/phy.h
> +++ b/include/dt-bindings/phy/phy.h
> @@ -17,5 +17,6 @@
>  #define PHY_TYPE_USB3		4
>  #define PHY_TYPE_UFS		5
>  #define PHY_TYPE_DP		6
> +#define PHY_TYPE_SGMII		7
>  
>  #endif /* _DT_BINDINGS_PHY */
>

Hi Kishon,

On Fri, Mar 13, 2020 at 04:44:04PM +0530, Kishon Vijay Abraham I wrote:
> +Rob

Any comment regarding patch 2/3 ? :-) You mentioned in your review of v5
that the exported symbols were a no-go, and that is now fixed. The
driver uses the PHY .configure() API to configure DisplayPort
parameters, .power_on() now waits for the PHY PLL to lock, and the
USB-specific exported symbols were removed with reset support being
moved to the PHY consumers (there's no reason for the PHY driver to
reset the PHY consumers, that's a layering violation).

> On 11/03/20 4:02 pm, Laurent Pinchart wrote:
> > From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > 
> > Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> > Processing System Gigabit Transceiver which provides PHY capabilities to
> > USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> > 
> > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v5:
> > 
> > - Document clocks and clock-names properties
> > - Document resets and reset-names properties
> > - Replace subnodes with an additional entry in the PHY cells
> > - Drop lane frequency PHY cell, replaced by reference clock phandle
> > - Convert bindings to YAML
> > - Reword the subject line
> > - Drop Rob's R-b as the bindings have significantly changed
> > - Drop resets and reset-names properties
> > ---
> >  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
> >  include/dt-bindings/phy/phy.h                 |   1 +
> >  2 files changed, 105 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > new file mode 100644
> > index 000000000000..9948e4a60e45
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > @@ -0,0 +1,104 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +
> > +description: |
> > +  This binding describes the Xilinx ZynqMP Gigabit Transceiver (GTR) PHY. The
> > +  GTR provides four lanes and is used by USB, SATA, PCIE, Display port and
> > +  Ethernet SGMII controllers.
> > +
> > +properties:
> > +  "#phy-cells":
> > +    const: 4
> > +    description: |
> > +      The cells contain the following arguments.
> > +
> > +      - description: The GTR lane
> > +        minimum: 0
> > +        maximum: 3
> > +      - description: The PHY type
> > +        enum:
> > +          - PHY_TYPE_DP
> > +          - PHY_TYPE_PCIE
> > +          - PHY_TYPE_SATA
> > +          - PHY_TYPE_SGMII
> > +          - PHY_TYPE_USB
> > +      - description: The PHY instance
> > +        minimum: 0
> > +        maximum: 1 # for DP, SATA or USB
> > +        maximum: 3 # for PCIE or SGMII
> > +      - description: The reference clock number
> > +        minimum: 0
> > +        maximum: 3
> > +
> > +  compatible:
> > +    enum:
> > +      - xlnx,zynqmp-psgtr-v1.1
> > +      - xlnx,zynqmp-psgtr
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 4
> > +    description: |
> > +      Clock for each PS_MGTREFCLK[0-3] reference clock input. Unconnected
> > +      inputs shall not have an entry.
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 4
> > +    items:
> > +      pattern: "^ref[0-3]$"
> > +
> > +  reg:
> > +    items:
> > +      - description: SERDES registers block
> > +      - description: SIOU registers block
> > +
> > +  reg-names:
> > +    items:
> > +      - const: serdes
> > +      - const: siou
> > +
> > +required:
> > +  - "#phy-cells"
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +
> > +if:
> > +  properties:
> > +    compatible:
> > +      const: xlnx,zynqmp-psgtr
> > +
> > +then:
> > +  properties:
> > +    xlnx,tx-termination-fix:
> > +      description: |
> > +        Include this for fixing functional issue with the TX termination
> > +        resistance in GT, which can be out of spec for the XCZU9EG silicon
> > +        version.
> > +      type: boolean
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    phy: phy@fd400000 {
> > +      compatible = "xlnx,zynqmp-psgtr-v1.1";
> > +      reg = <0x0 0xfd400000 0x0 0x40000>,
> > +            <0x0 0xfd3d0000 0x0 0x1000>;
> > +      reg-names = "serdes", "siou";
> > +      clocks = <&refclks 3>, <&refclks 2>, <&refclks 0>;
> > +      clock-names = "ref1", "ref2", "ref3";
> > +      #phy-cells = <4>;
> > +      status = "okay";
> > +    };
> > +
> > +...
> > diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
> > index 1f3f866fae7b..f6bc83b66ae9 100644
> > --- a/include/dt-bindings/phy/phy.h
> > +++ b/include/dt-bindings/phy/phy.h
> > @@ -17,5 +17,6 @@
> >  #define PHY_TYPE_USB3		4
> >  #define PHY_TYPE_UFS		5
> >  #define PHY_TYPE_DP		6
> > +#define PHY_TYPE_SGMII		7
> >  
> >  #endif /* _DT_BINDINGS_PHY */
> >

On Wed, Mar 11, 2020 at 12:32:50PM +0200, Laurent Pinchart wrote:
> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> 
> Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> Processing System Gigabit Transceiver which provides PHY capabilities to
> USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> 
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v5:
> 
> - Document clocks and clock-names properties
> - Document resets and reset-names properties
> - Replace subnodes with an additional entry in the PHY cells
> - Drop lane frequency PHY cell, replaced by reference clock phandle
> - Convert bindings to YAML
> - Reword the subject line
> - Drop Rob's R-b as the bindings have significantly changed
> - Drop resets and reset-names properties
> ---
>  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
>  include/dt-bindings/phy/phy.h                 |   1 +
>  2 files changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> new file mode 100644
> index 000000000000..9948e4a60e45
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> @@ -0,0 +1,104 @@
> +# SPDX-License-Identifier: GPL-2.0

For new bindings:

(GPL-2.0-only OR BSD-2-Clause)

Though I guess Anurag needs to agree.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +description: |
> +  This binding describes the Xilinx ZynqMP Gigabit Transceiver (GTR) PHY. The
> +  GTR provides four lanes and is used by USB, SATA, PCIE, Display port and
> +  Ethernet SGMII controllers.
> +
> +properties:
> +  "#phy-cells":
> +    const: 4
> +    description: |
> +      The cells contain the following arguments.
> +
> +      - description: The GTR lane
> +        minimum: 0
> +        maximum: 3
> +      - description: The PHY type
> +        enum:
> +          - PHY_TYPE_DP
> +          - PHY_TYPE_PCIE
> +          - PHY_TYPE_SATA
> +          - PHY_TYPE_SGMII
> +          - PHY_TYPE_USB
> +      - description: The PHY instance
> +        minimum: 0
> +        maximum: 1 # for DP, SATA or USB
> +        maximum: 3 # for PCIE or SGMII
> +      - description: The reference clock number
> +        minimum: 0
> +        maximum: 3

Humm, interesting almost json-schema. I guess it's fine as-is.

I would like to figure out how to apply a schema like this to the 
consumer nodes. We'd have to look up the phandle, get that node's 
compatible, find the provider's schema, find #.*-cells property, and 
extract a schema from it. Actually, doesn't sound too hard.

> +
> +  compatible:
> +    enum:
> +      - xlnx,zynqmp-psgtr-v1.1
> +      - xlnx,zynqmp-psgtr
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 4
> +    description: |
> +      Clock for each PS_MGTREFCLK[0-3] reference clock input. Unconnected
> +      inputs shall not have an entry.
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      pattern: "^ref[0-3]$"
> +
> +  reg:
> +    items:
> +      - description: SERDES registers block
> +      - description: SIOU registers block
> +
> +  reg-names:
> +    items:
> +      - const: serdes
> +      - const: siou
> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - reg
> +  - reg-names
> +
> +if:
> +  properties:
> +    compatible:
> +      const: xlnx,zynqmp-psgtr
> +
> +then:
> +  properties:
> +    xlnx,tx-termination-fix:
> +      description: |
> +        Include this for fixing functional issue with the TX termination
> +        resistance in GT, which can be out of spec for the XCZU9EG silicon
> +        version.
> +      type: boolean
> +
> +additionalProperties: false

This won't work with 'xlnx,tx-termination-fix'. You need to move it to 
the main properties section and then do:

if:
  properties:
    compatible:
      const: xlnx,zynqmp-psgtr-v1.1

then:
  properties:
    xlnx,tx-termination-fix: false

I think this would also work:

  not:
    required:
      - xlnx,tx-termination-fix
> +
> +examples:
> +  - |
> +    phy: phy@fd400000 {
> +      compatible = "xlnx,zynqmp-psgtr-v1.1";
> +      reg = <0x0 0xfd400000 0x0 0x40000>,
> +            <0x0 0xfd3d0000 0x0 0x1000>;
> +      reg-names = "serdes", "siou";
> +      clocks = <&refclks 3>, <&refclks 2>, <&refclks 0>;
> +      clock-names = "ref1", "ref2", "ref3";
> +      #phy-cells = <4>;
> +      status = "okay";

Drop status in examples.

> +    };
> +
> +...
> diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
> index 1f3f866fae7b..f6bc83b66ae9 100644
> --- a/include/dt-bindings/phy/phy.h
> +++ b/include/dt-bindings/phy/phy.h
> @@ -17,5 +17,6 @@
>  #define PHY_TYPE_USB3		4
>  #define PHY_TYPE_UFS		5
>  #define PHY_TYPE_DP		6
> +#define PHY_TYPE_SGMII		7
>  
>  #endif /* _DT_BINDINGS_PHY */
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi Rob,

On Thu, Mar 19, 2020 at 08:35:20PM -0600, Rob Herring wrote:
> On Wed, Mar 11, 2020 at 12:32:50PM +0200, Laurent Pinchart wrote:
> > From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > 
> > Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> > Processing System Gigabit Transceiver which provides PHY capabilities to
> > USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> > 
> > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v5:
> > 
> > - Document clocks and clock-names properties
> > - Document resets and reset-names properties
> > - Replace subnodes with an additional entry in the PHY cells
> > - Drop lane frequency PHY cell, replaced by reference clock phandle
> > - Convert bindings to YAML
> > - Reword the subject line
> > - Drop Rob's R-b as the bindings have significantly changed
> > - Drop resets and reset-names properties
> > ---
> >  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
> >  include/dt-bindings/phy/phy.h                 |   1 +
> >  2 files changed, 105 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > new file mode 100644
> > index 000000000000..9948e4a60e45
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > @@ -0,0 +1,104 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> For new bindings:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 
> Though I guess Anurag needs to agree.

There's an ongoing similar discussion regarding the DPSUB (DRM/KMS)
bindings. Hyun is checking with the Xilinx legal department. If they
agree, I'll change the license here, otherwise I'll keep it as-is.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +
> > +description: |
> > +  This binding describes the Xilinx ZynqMP Gigabit Transceiver (GTR) PHY. The
> > +  GTR provides four lanes and is used by USB, SATA, PCIE, Display port and
> > +  Ethernet SGMII controllers.
> > +
> > +properties:
> > +  "#phy-cells":
> > +    const: 4
> > +    description: |
> > +      The cells contain the following arguments.
> > +
> > +      - description: The GTR lane
> > +        minimum: 0
> > +        maximum: 3
> > +      - description: The PHY type
> > +        enum:
> > +          - PHY_TYPE_DP
> > +          - PHY_TYPE_PCIE
> > +          - PHY_TYPE_SATA
> > +          - PHY_TYPE_SGMII
> > +          - PHY_TYPE_USB
> > +      - description: The PHY instance
> > +        minimum: 0
> > +        maximum: 1 # for DP, SATA or USB
> > +        maximum: 3 # for PCIE or SGMII
> > +      - description: The reference clock number
> > +        minimum: 0
> > +        maximum: 3
> 
> Humm, interesting almost json-schema. I guess it's fine as-is.
> 
> I would like to figure out how to apply a schema like this to the 
> consumer nodes. We'd have to look up the phandle, get that node's 
> compatible, find the provider's schema, find #.*-cells property, and 
> extract a schema from it. Actually, doesn't sound too hard.

That would be nice :-)

> > +
> > +  compatible:
> > +    enum:
> > +      - xlnx,zynqmp-psgtr-v1.1
> > +      - xlnx,zynqmp-psgtr
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 4
> > +    description: |
> > +      Clock for each PS_MGTREFCLK[0-3] reference clock input. Unconnected
> > +      inputs shall not have an entry.
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 4
> > +    items:
> > +      pattern: "^ref[0-3]$"
> > +
> > +  reg:
> > +    items:
> > +      - description: SERDES registers block
> > +      - description: SIOU registers block
> > +
> > +  reg-names:
> > +    items:
> > +      - const: serdes
> > +      - const: siou
> > +
> > +required:
> > +  - "#phy-cells"
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +
> > +if:
> > +  properties:
> > +    compatible:
> > +      const: xlnx,zynqmp-psgtr
> > +
> > +then:
> > +  properties:
> > +    xlnx,tx-termination-fix:
> > +      description: |
> > +        Include this for fixing functional issue with the TX termination
> > +        resistance in GT, which can be out of spec for the XCZU9EG silicon
> > +        version.
> > +      type: boolean
> > +
> > +additionalProperties: false
> 
> This won't work with 'xlnx,tx-termination-fix'. You need to move it to 
> the main properties section and then do:
> 
> if:
>   properties:
>     compatible:
>       const: xlnx,zynqmp-psgtr-v1.1

It doesn't make a big difference as only two compatible values are
allowed, but is there a way to express the condition the other way
around, if (compatible != "xlnx,zynqmp-psgtr") ?

> 
> then:
>   properties:
>     xlnx,tx-termination-fix: false

This works.

> I think this would also work:
> 
>   not:
>     required:
>       - xlnx,tx-termination-fix

I've tested it and it works, but I'm not sure why, given that the
property isn't required required in the first place. Could you enlighten
me ?

> > +
> > +examples:
> > +  - |
> > +    phy: phy@fd400000 {
> > +      compatible = "xlnx,zynqmp-psgtr-v1.1";
> > +      reg = <0x0 0xfd400000 0x0 0x40000>,
> > +            <0x0 0xfd3d0000 0x0 0x1000>;
> > +      reg-names = "serdes", "siou";
> > +      clocks = <&refclks 3>, <&refclks 2>, <&refclks 0>;
> > +      clock-names = "ref1", "ref2", "ref3";
> > +      #phy-cells = <4>;
> > +      status = "okay";
> 
> Drop status in examples.

OK.

> > +    };
> > +
> > +...
> > diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
> > index 1f3f866fae7b..f6bc83b66ae9 100644
> > --- a/include/dt-bindings/phy/phy.h
> > +++ b/include/dt-bindings/phy/phy.h
> > @@ -17,5 +17,6 @@
> >  #define PHY_TYPE_USB3		4
> >  #define PHY_TYPE_UFS		5
> >  #define PHY_TYPE_DP		6
> > +#define PHY_TYPE_SGMII		7
> >  
> >  #endif /* _DT_BINDINGS_PHY */

On Fri, Mar 20, 2020 at 3:50 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Thu, Mar 19, 2020 at 08:35:20PM -0600, Rob Herring wrote:
> > On Wed, Mar 11, 2020 at 12:32:50PM +0200, Laurent Pinchart wrote:
> > > From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > >
> > > Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> > > Processing System Gigabit Transceiver which provides PHY capabilities to
> > > USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> > >
> > > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v5:
> > >
> > > - Document clocks and clock-names properties
> > > - Document resets and reset-names properties
> > > - Replace subnodes with an additional entry in the PHY cells
> > > - Drop lane frequency PHY cell, replaced by reference clock phandle
> > > - Convert bindings to YAML
> > > - Reword the subject line
> > > - Drop Rob's R-b as the bindings have significantly changed
> > > - Drop resets and reset-names properties
> > > ---
> > >  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
> > >  include/dt-bindings/phy/phy.h                 |   1 +
> > >  2 files changed, 105 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > > new file mode 100644
> > > index 000000000000..9948e4a60e45
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > > @@ -0,0 +1,104 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> >
> > For new bindings:
> >
> > (GPL-2.0-only OR BSD-2-Clause)
> >
> > Though I guess Anurag needs to agree.
>
> There's an ongoing similar discussion regarding the DPSUB (DRM/KMS)
> bindings. Hyun is checking with the Xilinx legal department. If they
> agree, I'll change the license here, otherwise I'll keep it as-is.

TBC, the choice is change it or take your toys elsewhere and play by
yourself. I don't really want to end up with whatever each submitter
desires. I don't expect there's many companies that object to a
permissive license.

> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > +
> > > +description: |
> > > +  This binding describes the Xilinx ZynqMP Gigabit Transceiver (GTR) PHY. The
> > > +  GTR provides four lanes and is used by USB, SATA, PCIE, Display port and
> > > +  Ethernet SGMII controllers.
> > > +
> > > +properties:
> > > +  "#phy-cells":
> > > +    const: 4
> > > +    description: |
> > > +      The cells contain the following arguments.
> > > +
> > > +      - description: The GTR lane
> > > +        minimum: 0
> > > +        maximum: 3
> > > +      - description: The PHY type
> > > +        enum:
> > > +          - PHY_TYPE_DP
> > > +          - PHY_TYPE_PCIE
> > > +          - PHY_TYPE_SATA
> > > +          - PHY_TYPE_SGMII
> > > +          - PHY_TYPE_USB
> > > +      - description: The PHY instance
> > > +        minimum: 0
> > > +        maximum: 1 # for DP, SATA or USB
> > > +        maximum: 3 # for PCIE or SGMII
> > > +      - description: The reference clock number
> > > +        minimum: 0
> > > +        maximum: 3
> >
> > Humm, interesting almost json-schema. I guess it's fine as-is.
> >
> > I would like to figure out how to apply a schema like this to the
> > consumer nodes. We'd have to look up the phandle, get that node's
> > compatible, find the provider's schema, find #.*-cells property, and
> > extract a schema from it. Actually, doesn't sound too hard.
>
> That would be nice :-)
>
> > > +
> > > +  compatible:
> > > +    enum:
> > > +      - xlnx,zynqmp-psgtr-v1.1
> > > +      - xlnx,zynqmp-psgtr
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    maxItems: 4
> > > +    description: |
> > > +      Clock for each PS_MGTREFCLK[0-3] reference clock input. Unconnected
> > > +      inputs shall not have an entry.
> > > +
> > > +  clock-names:
> > > +    minItems: 1
> > > +    maxItems: 4
> > > +    items:
> > > +      pattern: "^ref[0-3]$"
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: SERDES registers block
> > > +      - description: SIOU registers block
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: serdes
> > > +      - const: siou
> > > +
> > > +required:
> > > +  - "#phy-cells"
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > > +
> > > +if:
> > > +  properties:
> > > +    compatible:
> > > +      const: xlnx,zynqmp-psgtr
> > > +
> > > +then:
> > > +  properties:
> > > +    xlnx,tx-termination-fix:
> > > +      description: |
> > > +        Include this for fixing functional issue with the TX termination
> > > +        resistance in GT, which can be out of spec for the XCZU9EG silicon
> > > +        version.
> > > +      type: boolean
> > > +
> > > +additionalProperties: false
> >
> > This won't work with 'xlnx,tx-termination-fix'. You need to move it to
> > the main properties section and then do:
> >
> > if:
> >   properties:
> >     compatible:
> >       const: xlnx,zynqmp-psgtr-v1.1
>
> It doesn't make a big difference as only two compatible values are
> allowed, but is there a way to express the condition the other way
> around, if (compatible != "xlnx,zynqmp-psgtr") ?

if:
  properties:
    compatible:
      not:
        const: xlnx,zynqmp-psgtr

I think if: { not: ... } would also work. You'll have to test them out.

> > then:
> >   properties:
> >     xlnx,tx-termination-fix: false
>
> This works.
>
> > I think this would also work:
> >
> >   not:
> >     required:
> >       - xlnx,tx-termination-fix
>
> I've tested it and it works, but I'm not sure why, given that the
> property isn't required required in the first place. Could you enlighten
> me ?

'required' is true or false based on presence or absence of properties
in the list. If 'xlnx,tx-termination-fix' is present, then 'required'
evaluates to true. And the inverse is true. Then we take the inverse
of of that with 'not'.

The first case is what trips me up more because a property not present
evaluates to true. So if you look at 'select' schemas, we have to make
any properties we list (compatible typically) required.

Rob

Hi Rob,

On Fri, Mar 20, 2020 at 10:53:05AM -0600, Rob Herring wrote:
> On Fri, Mar 20, 2020 at 3:50 AM Laurent Pinchart wrote:
> > On Thu, Mar 19, 2020 at 08:35:20PM -0600, Rob Herring wrote:
> > > On Wed, Mar 11, 2020 at 12:32:50PM +0200, Laurent Pinchart wrote:
> > > > From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > > >
> > > > Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> > > > Processing System Gigabit Transceiver which provides PHY capabilities to
> > > > USB, SATA, PCIE, Display Port and Ehernet SGMII controllers.
> > > >
> > > > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > Changes since v5:
> > > >
> > > > - Document clocks and clock-names properties
> > > > - Document resets and reset-names properties
> > > > - Replace subnodes with an additional entry in the PHY cells
> > > > - Drop lane frequency PHY cell, replaced by reference clock phandle
> > > > - Convert bindings to YAML
> > > > - Reword the subject line
> > > > - Drop Rob's R-b as the bindings have significantly changed
> > > > - Drop resets and reset-names properties
> > > > ---
> > > >  .../bindings/phy/xlnx,zynqmp-psgtr.yaml       | 104 ++++++++++++++++++
> > > >  include/dt-bindings/phy/phy.h                 |   1 +
> > > >  2 files changed, 105 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > > > new file mode 100644
> > > > index 000000000000..9948e4a60e45
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml
> > > > @@ -0,0 +1,104 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > >
> > > For new bindings:
> > >
> > > (GPL-2.0-only OR BSD-2-Clause)
> > >
> > > Though I guess Anurag needs to agree.
> >
> > There's an ongoing similar discussion regarding the DPSUB (DRM/KMS)
> > bindings. Hyun is checking with the Xilinx legal department. If they
> > agree, I'll change the license here, otherwise I'll keep it as-is.
> 
> TBC, the choice is change it or take your toys elsewhere and play by
> yourself. I don't really want to end up with whatever each submitter
> desires. I don't expect there's many companies that object to a
> permissive license.

I don't expect that either, but it's out of my control in any case.
Let's say.

I've heard quite a few times that "the preferred license for new
bindings is GPL-2.0-only OR BSD-2-Clause", but this is the first time I
hear it's a hard requirement. I have missed the decision making process,
I have nothing to question, and I'll spread that message in the future.

> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > +
> > > > +description: |
> > > > +  This binding describes the Xilinx ZynqMP Gigabit Transceiver (GTR) PHY. The
> > > > +  GTR provides four lanes and is used by USB, SATA, PCIE, Display port and
> > > > +  Ethernet SGMII controllers.
> > > > +
> > > > +properties:
> > > > +  "#phy-cells":
> > > > +    const: 4
> > > > +    description: |
> > > > +      The cells contain the following arguments.
> > > > +
> > > > +      - description: The GTR lane
> > > > +        minimum: 0
> > > > +        maximum: 3
> > > > +      - description: The PHY type
> > > > +        enum:
> > > > +          - PHY_TYPE_DP
> > > > +          - PHY_TYPE_PCIE
> > > > +          - PHY_TYPE_SATA
> > > > +          - PHY_TYPE_SGMII
> > > > +          - PHY_TYPE_USB
> > > > +      - description: The PHY instance
> > > > +        minimum: 0
> > > > +        maximum: 1 # for DP, SATA or USB
> > > > +        maximum: 3 # for PCIE or SGMII
> > > > +      - description: The reference clock number
> > > > +        minimum: 0
> > > > +        maximum: 3
> > >
> > > Humm, interesting almost json-schema. I guess it's fine as-is.
> > >
> > > I would like to figure out how to apply a schema like this to the
> > > consumer nodes. We'd have to look up the phandle, get that node's
> > > compatible, find the provider's schema, find #.*-cells property, and
> > > extract a schema from it. Actually, doesn't sound too hard.
> >
> > That would be nice :-)
> >
> > > > +
> > > > +  compatible:
> > > > +    enum:
> > > > +      - xlnx,zynqmp-psgtr-v1.1
> > > > +      - xlnx,zynqmp-psgtr
> > > > +
> > > > +  clocks:
> > > > +    minItems: 1
> > > > +    maxItems: 4
> > > > +    description: |
> > > > +      Clock for each PS_MGTREFCLK[0-3] reference clock input. Unconnected
> > > > +      inputs shall not have an entry.
> > > > +
> > > > +  clock-names:
> > > > +    minItems: 1
> > > > +    maxItems: 4
> > > > +    items:
> > > > +      pattern: "^ref[0-3]$"
> > > > +
> > > > +  reg:
> > > > +    items:
> > > > +      - description: SERDES registers block
> > > > +      - description: SIOU registers block
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: serdes
> > > > +      - const: siou
> > > > +
> > > > +required:
> > > > +  - "#phy-cells"
> > > > +  - compatible
> > > > +  - reg
> > > > +  - reg-names
> > > > +
> > > > +if:
> > > > +  properties:
> > > > +    compatible:
> > > > +      const: xlnx,zynqmp-psgtr
> > > > +
> > > > +then:
> > > > +  properties:
> > > > +    xlnx,tx-termination-fix:
> > > > +      description: |
> > > > +        Include this for fixing functional issue with the TX termination
> > > > +        resistance in GT, which can be out of spec for the XCZU9EG silicon
> > > > +        version.
> > > > +      type: boolean
> > > > +
> > > > +additionalProperties: false
> > >
> > > This won't work with 'xlnx,tx-termination-fix'. You need to move it to
> > > the main properties section and then do:
> > >
> > > if:
> > >   properties:
> > >     compatible:
> > >       const: xlnx,zynqmp-psgtr-v1.1
> >
> > It doesn't make a big difference as only two compatible values are
> > allowed, but is there a way to express the condition the other way
> > around, if (compatible != "xlnx,zynqmp-psgtr") ?
> 
> if:
>   properties:
>     compatible:
>       not:
>         const: xlnx,zynqmp-psgtr
> 
> I think if: { not: ... } would also work. You'll have to test them out.

I tried both, and neither worked. No big deal, I'll keep the current
expression.

> > > then:
> > >   properties:
> > >     xlnx,tx-termination-fix: false
> >
> > This works.
> >
> > > I think this would also work:
> > >
> > >   not:
> > >     required:
> > >       - xlnx,tx-termination-fix
> >
> > I've tested it and it works, but I'm not sure why, given that the
> > property isn't required required in the first place. Could you enlighten
> > me ?
> 
> 'required' is true or false based on presence or absence of properties
> in the list. If 'xlnx,tx-termination-fix' is present, then 'required'
> evaluates to true. And the inverse is true. Then we take the inverse
> of of that with 'not'.
> 
> The first case is what trips me up more because a property not present
> evaluates to true. So if you look at 'select' schemas, we have to make
> any properties we list (compatible typically) required.