Re: [PATCH net-next v2 2/9] dt-bindings: net: add backplane dt bindings

[Rob Herring <robh+dt@kernel.org>]

On Fri, Apr 24, 2020 at 7:46 AM Florinel Iordache
<florinel.iordache@nxp.com> wrote:
>
> Add ethernet backplane device tree bindings

For a new, common binding, you've got to do better than this. Bindings
need to stand on their own. I need a h/w block diagram or something
because I know little about "ethernet backplane".

>
> Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com>
> ---
>  .../bindings/net/ethernet-controller.yaml          |  3 +-
>  .../devicetree/bindings/net/ethernet-phy.yaml      | 50 +++++++++++++++++++++
>  .../devicetree/bindings/net/serdes-lane.yaml       | 51 ++++++++++++++++++++++
>  Documentation/devicetree/bindings/net/serdes.yaml  | 44 +++++++++++++++++++
>  4 files changed, 147 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/net/serdes-lane.yaml
>  create mode 100644 Documentation/devicetree/bindings/net/serdes.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index ac471b6..541cee5 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -93,8 +93,9 @@ properties:
>        - rxaui
>        - xaui
>
> -      # 10GBASE-KR, XFI, SFI
> +      # 10GBASE-KR, 40GBASE-KR4, XFI, SFI
>        - 10gbase-kr
> +      - 40gbase-kr4
>        - usxgmii
>
>    phy-mode:
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 5aa141c..436b5a7 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -161,6 +161,42 @@ properties:
>      description:
>        Specifies a reference to a node representing a SFP cage.
>
> +  eq-algorithm:
> +    description:
> +      Specifies the desired equalization algorithm to be used
> +      by the KR link training
> +    oneOf:
> +      - const: fixed
> +        description:
> +          Backplane KR using fixed coefficients meaning no
> +          equalization algorithm
> +      - const: bee
> +        description:
> +          Backplane KR using 3-Taps Bit Edge Equalization (BEE)
> +          algorithm
> +
> +  eq-init:

eq-coefficients?

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 3
> +    maxItems: 3
> +    description:
> +      Triplet of KR coefficients. Specifies the initialization
> +      values for standard KR equalization coefficients used by
> +      the link training (pre-cursor, main-cursor, post-cursor)

items:
  - description: pre-cursor
  - description: main-cursor
  ...

Is 0-2^32 valid data? If not, add some constraints.

> +
> +  eq-params:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      Variable size array of KR parameters. Specifies the HW
> +      specific parameters used by the link training.

DT is not a dumping ground for magic register values.

eq-init vs. eq-params is pretty vague as to what they are.

I fail to see how these properties are related to $subject. Should be
a separate patch.

> +
> +  lane-handle:
> +    $ref: /schemas/types.yaml#definitions/phandle
> +    description:
> +      Specifies a reference (or array of references) to a node
> +      representing the desired SERDES lane (or lanes) used in
> +      backplane mode.
> +
>  required:
>    - reg
>
> @@ -183,3 +219,17 @@ examples:
>              reset-deassert-us = <2000>;
>          };
>      };
> +  - |
> +    ethernet {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@0 {
> +            compatible = "ethernet-phy-ieee802.3-c45";
> +            reg = <0x0>;
> +            lane-handle = <&lane_d>;
> +            eq-algorithm = "fixed";
> +            eq-init = <0x2 0x29 0x5>;
> +            eq-params = <0>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/net/serdes-lane.yaml b/Documentation/devicetree/bindings/net/serdes-lane.yaml
> new file mode 100644
> index 0000000..ce3581e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/serdes-lane.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings:

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

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/serdes-lane.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Serdes Lane Binding
> +
> +maintainers:
> +  - Florinel Iordache <florinel.iordache@nxp.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "^lane(@[a-f0-9]+)?$"
> +
> +  compatible:
> +    oneOf:
> +      - const: lane-10g
> +        description: Lane part of a 10G SerDes module
> +      - const: lane-28g
> +        description: Lane part of a 28G SerDes module
> +
> +  reg:
> +    description:
> +      Registers memory map offset and size for this lane
> +
> +  reg-names:
> +    description:
> +      Names of the register map given in "reg" node.
> +
> +examples:
> +  - |
> +    serdes1: serdes@1ea0000 {
> +        compatible = "serdes-10g";

Do you have a datasheet for this device as bindings describe devices?
I assume not because serdes is a protocol, not a device. AFAIK,
there's no standard programming interface for 'serdes' as that is
about the only time we have any sort of genericish compatible strings.
The compatible string at a minimum should tell me what the programming
model for the registers are.

> +        reg = <0x0 0x1ea0000 0 0x00002000>;
> +        reg-names = "serdes", "serdes-10g";

The default address and sizes are 1 cell. So you have addr 0 with size
0x1ea0000 and then addr 0 with size 0x2000.

> +        little-endian;
> +
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        lane_a: lane@800 {
> +            compatible = "lane-10g";
> +            reg = <0x800 0x40>;
> +            reg-names = "lane", "serdes-lane";

Not valid. You have 1 entry (with a addr and size) for reg, but 2
entries for reg-names.

40G is made up of 4 10G lanes, right? Do all 40G serdes phys have
separate register regions for each lane? You can't assume lanes and DT
nodes are 1-1.

As lanes have to be child nodes, these 2 schemas should be 1. Though I
don't think lane nodes will survive.

> +        };
> +        lane_b: lane@840 {
> +            compatible = "lane-10g";
> +            reg = <0x840 0x40>;
> +            reg-names = "lane", "serdes-lane";
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/net/serdes.yaml b/Documentation/devicetree/bindings/net/serdes.yaml
> new file mode 100644
> index 0000000..fd3da85
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/serdes.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/serdes.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Serdes Module Binding
> +
> +maintainers:
> +  - Florinel Iordache <florinel.iordache@nxp.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "^serdes(@[a-f0-9]+)?$"
> +
> +  compatible:
> +    oneOf:
> +      - const: serdes-10g
> +        description: SerDes module type of 10G
> +      - const: serdes-28g
> +        description: SerDes module type of 28G
> +
> +  reg:
> +    description:
> +      Registers memory map offset and size for this serdes module
> +
> +  reg-names:
> +    description:
> +      Names of the register map given in "reg" node.
> +
> +  little-endian:
> +    description:
> +      Specifies the endianness of serdes module
> +      For complete definition see
> +      Documentation/devicetree/bindings/common-properties.txt
> +
> +examples:
> +  - |
> +    serdes1: serdes@1ea0000 {
> +        compatible = "serdes-10g";
> +        reg = <0x0 0x1ea0000 0 0x00002000>;
> +        reg-names = "serdes", "serdes-10g";
> +        little-endian;
> +    };
> --
> 1.9.1
>