Re: [RFC net-next PATCH 01/13] dt-bindings: net: Add binding for Xilinx PCS

[Krzysztof Kozlowski]

On Thu, Apr 03, 2025 at 02:18:55PM GMT, Sean Anderson wrote:
> This adds a binding for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII

Incomplete review, since this is an RFC.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> LogiCORE IP. This device is a soft device typically used to adapt
> between GMII and SGMII or 1000BASE-X (possbilty in combination with a
> serdes). pcs-modes reflects the modes available with the as configured
> when the device is synthesized. Multiple modes may be specified if
> dynamic reconfiguration is supported.
> 
> One PCS may contain "shared logic in core" which can be connected to
> other PCSs with "shared logic in example design." This primarily refers
> to clocking resources, allowing a reference clock to be shared by a bank
> of PCSs. To support this, if #clock-cells is defined then the PCS will
> register itself as a clock provider for other PCSs.
> 
> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
> ---
> 
>  .../devicetree/bindings/net/xilinx,pcs.yaml   | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/xilinx,pcs.yaml b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml
> new file mode 100644
> index 000000000000..56a3ce0c4ef0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/xilinx,pcs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE IP
> +
> +maintainers:
> +  - Sean Anderson <sean.anderson@xxxxxxxx>
> +
> +description:
> +  This is a soft device which implements the PCS and (depending on
> +  configuration) PMA layers of an IEEE Ethernet PHY. On the MAC side, it
> +  implements GMII. It may have an attached SERDES (internal or external), or
> +  may directly use LVDS IO resources. Depending on the configuration, it may
> +  implement 1000BASE-X, SGMII, 2500BASE-X, or 2.5G SGMII.
> +
> +  This device has a notion of "shared logic" such as reset and clocking
> +  resources which must be shared between multiple PCSs using the same I/O
> +  banks. Each PCS can be configured to have the shared logic in the "core"
> +  (instantiated internally and made available to other PCSs) or in the "example
> +  design" (provided by another PCS). PCSs with shared logic in the core are
> +  reset controllers, and generally provide several resets for other PCSs in the
> +  same bank.
> +
> +allOf:
> +  - $ref: ethernet-controller.yaml#
> +
> +properties:
> +  compatible:
> +    contains:

>From where did you get such syntax? What do you want to express?

> +      const: xilinx,pcs-16.2

What does the number mean?

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    const: 0
> +    description:
> +      Register a clock representing the clocking resources shared with other
> +      PCSs.

Drop description, redundant.

> +
> +  clocks:
> +    items:
> +      - description:
> +          The reference clock for the PCS. Depending on your setup, this may be
> +          the gtrefclk, refclk, clk125m signal, or clocks from another PCS.
> +
> +  clock-names:
> +    const: refclk
> +
> +  done-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO connected to the reset-done output, if present.
> +
> +  interrupts:
> +    items:
> +      - description:
> +          The an_interrupt autonegotiation-complete interrupt.
> +
> +  interrupt-names:
> +    const: an
> +
> +  pcs-modes:
> +    description:
> +      The interfaces that the PCS supports.
> +    oneOf:
> +      - const: sgmii
> +      - const: 1000base-x
> +      - const: 2500base-x
> +      - items:
> +          - const: sgmii
> +          - const: 1000base-x

This is confusing. Why fallbacks? Shouldn't this be just enum? And
where is the type or constraints about number of items?

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO connected to the reset input.
> +
> +required:
> +  - compatible
> +  - reg
> +  - pcs-modes
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pcs0: ethernet-pcs@0 {
> +            #clock-cells = <0>;

Follow DTS coding style. clock-cells are never the first property.

> +            compatible = "xlnx,pcs-16.2";
> +            reg = <0>;
> +            clocks = <&si570>;
> +            clock-names = "refclk";
> +            interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "an";
> +            reset-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> +            done-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> +            pcs-modes = "sgmii", "1000base-x";
> +        };
> +
> +        pcs1: ethernet-pcs@1 {
> +            compatible = "xlnx,pcs-16.2";
> +            reg = <1>;
> +            clocks = <&pcs0>;
> +            clock-names = "refclk";
> +            interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "an";
> +            reset-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>;
> +            done-gpios = <&gpio 8 GPIO_ACTIVE_HIGH>;
> +            pcs-modes = "sgmii", "1000base-x";

Drop example, basically the same as previous.

Best regards,
Krzysztof